[poppler] Object management helper class

[Albert Astals Cid]

El diumenge, 5 de juny de 2016, a les 10:44:56 CEST, Carlos Garcia Campos va 
escriure:
> 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.


That's LibreOffice, as far as i know there's more users of poppler core.

Cheers,
  Albert