[poppler] Object management helper class

[Carlos Garcia Campos]

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

> El dimecres, 1 de juny de 2016, a les 18:07:44 CEST, Carlos Garcia Campos va 
> escriure:
>> Albert Astals Cid <aacid at kde.org> writes:
>> > El dimarts, 31 de maig de 2016, a les 11:46:38 CEST, Carlos Garcia Campos
>> > va> 
>> > escriure:
>> >> 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.
>> > 
>> > I did work a bit on this (your C++11 suggestions) and it's doable, it's a
>> > bit tricky in Stream where all the Stream seem to share the same Dict by
>> > virtue of assigning it around, but i think i'd make it work eventually.
>> 
>> We could even remove copy() and make = always copy. Dicts are refcounted
>> anyway, so copies should be cheap in any case. But I haven't looked at it in
>> detail yet, just proposed ideas without actually trying anything :-P
>> > There's a problem though, doing that we expose C++11 in the "internal"
>> > API,
>> > and I'm not sure the consumers of that internal API will be happy.
>> > 
>> > In theory we've said multiple times we don't care, that people should be
>> > using one of the frontends, but that just doesn't happen, and if we break
>> > the API in a way say libreoffice or latex can't use us anymore we
>> > basically make upgrading poppler almost impossible for distros.
>> > 
>> > So i will now try to do it the way I suggested (that is with some more
>> > manual C++), I suggest we start by limiting C++11 to the .cc files for
>> > now.
>> > 
>> > Am I making sense?
>> 
>> I would ask LO and pdflatex devs first, maybe it's not a problem at all
>> for them.
>
> So you're volunteering? ;)

Not really :-P LO is already using C++ 11, at least a subset of it, what
GCC 4.7 supports, which is more than enough for us. For other projects
using our core API, it would be a matter of adding a compile option, and
if we follow LO and we don't require GCC > 4.7, it shouldn't be a big
deal for anybody.

> Cheers,
>   Albert
>
>> 
>> > Cheers,
>> > 
>> >   Albert
>> >   
>> >> > 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/20160605/fd04986a/attachment.sig>