[poppler] Object management helper class

[Albert Astals Cid]

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? ;)

Cheers,
  Albert

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