[poppler] Object management helper class

[Carlos Garcia Campos]

Albert Astals Cid <aacid at kde.org> writes:

> El dilluns, 30 de maig de 2016, a les 15:43:19 CEST, Carlos Garcia Campos va 
> escriure:
>> Albert Astals Cid <aacid at kde.org> writes:
>> > Hi guys, related to the previous email about std::unique_ptr I think we
>> > would greatly benefit of a class that makes Object management easier.
>> > 
>> > Again if you go and check https://bugs.freedesktop.org/attachment.cgi?
>> > id=124163 we're missing lots of free() and it's hard to prove we won't
>> > miss
>> > more.
>> > 
>> > My suggestion is adding a class called UniqueObject (better name welcome)
>> > 
>> > which will:
>> >  * Call free on itself when it goes out of scope
>> >  
>> > 	So we don't need to add .free() in every other if-chek-error-return
>> > 	
>> >  * Call free on itself when you write on it
>> >  
>> > 	So we can do
>> > 	
>> > 	dict->lookup("Decode", &obj1);
>> > 	
>> >     if (obj1.isNull()) {
>> >     
>> >       dict->lookup("D", &obj1);
>> >     
>> >     }
>> > 
>> > instead of
>> > 
>> > 	dict->lookup("Decode", &obj1);
>> > 	
>> >     if (obj1.isNull()) {
>> >     
>> >       obj1.free();
>> >       dict->lookup("D", &obj1);
>> >     
>> >     }
>> > 
>> > What do you think?
>> 
>> Why do we need a new class? Wouldn't it be enough to call free in the
>> Object destructor and init* methods?
>
> Not really enough, we need to fix shallowCopy and the operator= that is used 
> randomly in the code, and also the whole rest of the code.
>
> I agree this may be the best, to not have two "Object" systems at the same 
> time, but it's also a very big change that needs removing of all the free() 
> calls (well not really since gree of a free object does nothing but still 
> would look weird).
>
> If we wanted to do this i guess we would do: 
>  * add destructor with free
>  * call free in the initObj define
>  * Rename shallowCopy to be like "takeObject" that would take the internal 
> data and set the other objcet to none

Or better remove shallowCopy and use std::move()

>  * Make operator= private so people are forced to use takeObject

Or make the object non-copyable by defining the copy constructor and
operator= as =delete.

>  * Make free() private so we can easily remove all its uses
>
> And maybe that would be all?

It seems so

> Sounds doable-ish?

Definitely.

> Cheers,
>   Albert
>
>> 
>> > Cheers,
>> > 
>> >   Albert
>> > 
>> > _______________________________________________
>> > poppler mailing list
>> > poppler at lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/poppler
>
>

-- 
Carlos Garcia Campos
PGP key: http://pgp.mit.edu:11371/pks/lookup?op=get&search=0x523E6462
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 180 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/poppler/attachments/20160531/775770dd/attachment.sig>