[poppler] splash/SplashFontFile.h

[Albert Astals Cid]

A Dimarts, 2 de novembre de 2010, Vincent Torri va escriure:
> On Mon, 1 Nov 2010, Albert Astals Cid wrote:
> > splash/SplashFontFile.h |    5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > New commits:
> > commit 6751eb48dc49890f7ad8b732b3fc29a2db746ec4
> > Author: Albert Astals Cid <aacid at kde.org>
> > Date:   Tue Nov 2 00:26:08 2010 +0000
> > 
> >    Make the destructor private
> >    
> >    You are not supposed to call it, you should call unref
> 
> then, why do you redefine it ? 

Redefine what? The destructor? Because we need it

> It is very rare (and imho very dangerous) to explicitely call the 
destructor, btw.

Rare calling a destructor in general? Or you mean doing "delete this"?

Doing "delete this" is perfectly fine and non dangerous (altough i agree it's 
rare for non explicitely refcounted classes) if the semantics are well 
defined. We do it in lots of other classes. That's why the destructor has to 
be private so you can see the semantics of ref/unref. (And yes i agree that 
explicitely refcounted classes suck and implicitely refcounted ones are much 
better/nicer to use)

> Also, I note that it deletes an object that is not allocated in the
> constructor. It should not. It's up to the user to destroy idA which is
> passed to the constructor, not the destructor.

That's your opinion, not what whoever coded the class (Takashi afair) decided.

It is quite normal adopting pointers you are passed either in the constructor 
or in some method. The thing is that it is usually documented, but this is 
poppler so sadly there is no documentation.

Albert

> 
> Vincent Torri
> 
> > diff --git a/splash/SplashFontFile.h b/splash/SplashFontFile.h
> > index f2b4570..8945be2 100644
> > --- a/splash/SplashFontFile.h
> > +++ b/splash/SplashFontFile.h
> > @@ -12,7 +12,7 @@
> > // under GPL version 2 or later
> > //
> > // Copyright (C) 2006 Takashi Iwai <tiwai at suse.de>
> > -// Copyright (C) 2008 Albert Astals Cid <aacid at kde.org>
> > +// Copyright (C) 2008, 2010 Albert Astals Cid <aacid at kde.org>
> > //
> > // To see a description of the changes please see the Changelog file that
> > // came with your tarball or type make ChangeLog if you are building from
> > git @@ -41,7 +41,6 @@ class SplashFontFileID;
> > class SplashFontSrc {
> > 
> > public:
> >   SplashFontSrc();
> > 
> > -  ~SplashFontSrc();
> > 
> >   void setFile(GooString *file, GBool del);
> >   void setFile(const char *file, GBool del);
> > 
> > @@ -56,6 +55,8 @@ public:
> >   int bufLen;
> >   GBool deleteSrc;
> >   int refcnt;
> > 
> > +private:
> > +  ~SplashFontSrc();
> > };
> > 
> > class SplashFontFile {
> > _______________________________________________
> > poppler mailing list
> > poppler at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/poppler
> 
> _______________________________________________
> poppler mailing list
> poppler at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/poppler