[poppler] splash/SplashFontFile.h

[Albert Astals Cid]

A Dimarts, 2 de novembre de 2010, vàreu escriure:
> On Tue, 2 Nov 2010, Albert Astals Cid wrote:
> > 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
> 
> why ?
> 
> >> 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"?
> 
> to *explicitely* call the destructor. The destructor is automatically
> called at the end of the block where the var has been declared, or when
> delete it if it has been allocated by new.
> 
> > 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.
> 
> It is dangerous if the guys does not know what he is doing, which is
> quite possible in C++. Then a double free can be done.

clueless people do wrong things all the time.

> 
> > 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.
> 
> Yes, it's my opinion. My opinion is that the class should own its data.
> It's a safer opinion. Less possible leaks.
> 
> But it seems you are stuck with a code you didn't write.

I'm stuck with a code that works ;-)

Albert

> 
> Vincent Torri
> 
> > 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
> > 
> > _______________________________________________
> > poppler mailing list
> > poppler at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/poppler