[poppler] splash/SplashFontFile.h

[Vincent Torri]

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.

> 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.

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
>
>