Re: [Pdfedit-devel] CPdf::getInstance

[Michal H. <ms...@gm...>]

On Sat, Feb 09, 2008 at 09:10:03AM +0000, Jozef Misutka wrote:
> > Date: Fri, 8 Feb 2008 21:11:11 +0100> From: ms...@gm...> To: pdf...@li...> CC: mis...@ho...> Subject: Re: [Pdfedit-devel] CPdf::getInstance> > On Thu, Feb 07, 2008 at 01:13:53PM +0100, Michal Hocko wrote:> [...]> > > >> > > > cpdf.cc:> > > >> > > > namespace {> > > > struct pdf_deleter> > > > {> > > > FILE* _fp;> > > > pdf_deleter () : _fp(NULL) {}> > > > pdf_deleter (FILE* fp) : _fp(fp) {}> > > > void operator() (CPdf* p)> > > > {> > > > assert (p);> > > > p->close();> > > > if (_fp)> > > > fclose (_fp);> > > > }> > > > };> > > > }> > > >> > > > boost::shared_ptr<CPdf> CPdf::getInstance(const char * filename,> > > OpenMode mode)> > > > {> > > > ...> > > > FILE * file=fopen(filename, openMode);> > > > ...> > > > return boost::shared_ptr<CPdf> (instance, pdf_deleter(file));> > > > ...> > > > }> > >> > > This seems reasonable. Could you send the patch and commit to the trunk?> > >> > > > I am still working on this solution but it needs some hacking to be> > accomplished (e.g. getIndirectProperty calls IProperty::setPdf(this).> > Note that once we change getInstance we have to change also> > IProperty::{get,set}Pdf methods and thus wrap this with shared> > ptr too. This is not very nice and has to be done very carefully).> > > > In the meantime we can use attached patch, which basically stores> > file handle directly to CPdf.> > Index: src/kernel/cpdf.cc> > ===================================================================> > RCS file: /cvsroot/pdfedit/pdfedit/src/kernel/cpdf.cc,v> > retrieving revision 1.91> > diff -u -p -r1.91 cpdf.cc> > --- src/kernel/cpdf.cc 31 Jan 2008 21:54:03 -0000 1.91> > +++ src/kernel/cpdf.cc 7 Feb 2008 12:01:24 -0000> > @@ -2197,6 +2197,7 @@ using namespace std;> > try> > {> > CPdf * instance=new CPdf(stream, mode);> > + instance->file = file;> > kernelPrintDbg(debug::DBG_INFO, "Instance created successfully openMode=" << openMode);> > return instance;> > }catch(exception &e)> > @@ -2217,7 +2218,14 @@ int CPdf::close(bool saveFlag)> > > > // deletes this instance> > // all clean-up is made in destructor> > + FILE * f = file;> > delete this;> > + if(fclose(f))> > + {> > + int err = errno;> > + kernelPrintDbg(debug::DBG_ERR, "Unable to close file handle (cause=\""> > + <<strerror(err) << "\"");> > + }> > > > kernelPrintDbg(debug::DBG_INFO, "Instance deleted.")> > return 0;> > Index: src/kernel/cpdf.h> > ===================================================================> > RCS file: /cvsroot/pdfedit/pdfedit/src/kernel/cpdf.h,v> > retrieving revision 1.85> > diff -u -p -r1.85 cpdf.h> > --- src/kernel/cpdf.h 18 Jan 2008 22:24:56 -0000 1.85> > +++ src/kernel/cpdf.h 7 Feb 2008 12:01:25 -0000> > @@ -776,6 +776,13 @@ protected:> > */> > IndiRef subsReferencies(boost::shared_ptr<IProperty> ip, ResolvedRefStorage & container, bool followRefs);> > private:> > +> > + /** File handle for document.> > + * Initialized in getInstance and may be used *ONLY* in close method (to > > + * close). All other operations must be done on top of stream object > > + * which wrapps file handle.> > + */> > + FILE * file;> > > > /** Identificator for this pdf instance.> > */> > This approach was used for bug 201 closing. shared_ptr solution is> neither better nor nicer. Problem with IProperty::{set,get}Pdf seems to> have no good solution because:> * all users of CPdf have to use shared_ptr<CPdf> instead of CPdf
>  
> - this is an *ADVANTAGE* (rather smart pointers than raw pointers)

I didn't mean it like disadvantage. I just wanted to emphasise that.

>  
> > * If we keep CPdf * inside IProperty then we will lose advantage> of shared_ptr<CPdf> because we can loose the last reference> while properties still exist.
> well, i really didn't understand this.. we keep CPdf* in IProperty.... see below
>  
> class IProperty : public IPropertyObserverSubject
> {
> private:
> IndiRef ref; /**< Objects pdf identification and generation number. */
> PropertyMode mode; /**< Mode of this property. */
> CPdf* pdf; /**< This object belongs to this pdf. */ 
> bool wantDispatch;/**< If true changes are dispatched. */

Yes we do. I just wanted to tell, that if we want to *keep* this than we
have some problems later (as described below).

>  
> > * If we want shared_ptr<CPdf> inside IProperty then we have> problem in CPdf::getIndirectProperty where> utils::createObjFromXpdfObj is used (it gets pdf parameter).> So we need effective way to wrap this in shared_ptr. This can
>  
> there is another far more serious problem - cyclic smart pointer reference, CPdf would never get deallocated and that is why we use CPdf* in IProperty not a smart pointer

Ok. Then (if we keep CPdf * in IProperty) we have a problem that CPdf may 
go deallocated while we still keep pointers in properties (so shared_ptr
for CPdf doesn't do its job).

> > be done either by fake shared_ptr (one which doesn't> deallocate its object when no references exist) or that we> store shared_ptr<CPdf>(this) inside CPdf. The later option is> really ugly hack, because then we will have artificial> reference to the CPdf instance which is never released> directly so some method has to be called to do it (e.g. close)> method. Besides that there are too many changes which have to> be done (see attached patch which is compilable but not> working at the moment).> > So that I don't see proper solution with shared_ptr. Jozef, please try> to look at it and give some feedback, maybe I am just overlooking> something. > 
>  
> My opinion is, that it is a clean solution regarding the current state-of-art of pdfedit.. 
> and it is definitely working because i use it in my local repository.

Have you reviewed the patch? Is there some difference with your
solution? 
I am not saying that this is *not* working or a bad solution. 
I have said that my patch still have some problems with asserts 
(that's why I asked for review). When you have proposed this solution I
was very happy with it. When I tried to implement it I found a lot of
problems, though.
How did you solved problem with shared_ptr<CPdf> inside IProperty?


>  
> jozo

P.S.
Please try to do something with you email client. It is almost impossible 
to do inline answering because all the text you are replying to is put
into the one huge line (see the begging of this email). I don't want to
force/push you to do anything, consider it according what you do think
is good.
-- 
Michal Hocko