[poppler] Object management helper class

[Albert Astals Cid]

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.

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?

Cheers,
  Albert

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