[poppler] Branch 'better_object' - cpp/poppler-document.cpp cpp/poppler-page.cpp glib/poppler-action.cc glib/poppler-attachment.cc glib/poppler-document.cc glib/poppler-input-stream.cc glib/poppler-input-stream.h glib/poppler-movie.cc glib/poppler-page.cc glib/poppler-structure-element.cc poppler/Annot.cc poppler/Annot.h poppler/Array.cc poppler/Array.h poppler/CairoFontEngine.cc poppler/CairoOutputDev.cc poppler/Catalog.cc poppler/Catalog.h poppler/CMap.cc poppler/DCTStream.cc poppler/Dict.cc poppler/Dict.h poppler/FileSpec.cc poppler/FileSpec.h poppler/FontInfo.cc poppler/Form.cc poppler/Form.h poppler/Function.cc poppler/Gfx.cc poppler/GfxFont.cc poppler/GfxFont.h poppler/Gfx.h poppler/GfxState.cc poppler/Hints.cc poppler/JBIG2Stream.cc poppler/JPEG2000Stream.cc poppler/Lexer.cc poppler/Lexer.h poppler/Linearization.cc poppler/Link.cc poppler/Movie.cc poppler/Movie.h poppler/Object.cc poppler/Object.h poppler/OptionalContent.cc poppler/Outline.cc poppler/Page.cc poppler/Page.h popp ler/PageLabelInfo.cc poppler/PageTransition.cc poppler/Parser.cc poppler/Parser.h poppler/PDFDoc.cc poppler/PDFDoc.h poppler/PopplerCache.cc poppler/PopplerCache.h poppler/PSOutputDev.cc poppler/Rendition.cc poppler/SecurityHandler.cc poppler/Sound.cc poppler/Sound.h poppler/SplashOutputDev.cc poppler/StdinPDFDocBuilder.cc poppler/Stream.cc poppler/Stream.h poppler/StructElement.cc poppler/StructTreeRoot.cc poppler/ViewerPreferences.cc poppler/XRef.cc poppler/XRef.h qt4/src qt4/tests qt5/src qt5/tests test/pdf-fullrewrite.cc utils/pdfinfo.cc utils/pdftohtml.cc utils/pdftotext.cc utils/pdfunite.cc

Carlos Garcia Campos carlosgc at gnome.org
Sat May 13 07:25:30 UTC 2017

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

> El divendres, 12 de maig de 2017, a les 15:31:55 CEST, Carlos Garcia Campos va 
> escriure:
>> Albert Astals Cid <aacid at kemper.freedesktop.org> writes:
>> This looks great, I have a couple of comments after a first glance.
>> >  	  if (!found)
>> > 
>> > -	    annotObj.free ();
>> > +	    annotObj.setToNull ();
>> As I commented on IRC, I think
>> annotObj = Object(objNull);
>> is not less efficient, looks simpler and we don't need setToNull() at
>> all.
>> > diff --git a/poppler/Annot.cc b/poppler/Annot.cc
>> > index 73c2fe66..96a7e330 100644
>> > --- a/poppler/Annot.cc
>> > +++ b/poppler/Annot.cc
>> > 
>> > @@ -1228,12 +1188,13 @@ Annot::Annot(PDFDoc *docA, Dict *dict, Object
>> > *obj) {> 
>> >    }
>> >    flags = flagUnknown;
>> >    type = typeUnknown;
>> > 
>> > -  annotObj.initDict (dict);
>> > +  dict->incRef();
>> > +  annotObj = Object(dict);
>> > 
>> >    initialize (docA, dict);
>> I still find this pattern quite confusing. The problem is that Object()
>> is taking the ownership of passing refcounted objects. That's not
>> consistent with all other types where the value is copied. I think it
>> should be Object() constructor the one doing the incRef() in case of
>> dicts, arrays and streams.
> You just complained exactly about the reverse thing (needing to do a decRef 
> when passing a Dict to an Object) in the other email ;)

Yes, because it was done before, not after. I think what makes poppler
refcounted objects confusing is the fact thet they don't delete
themselves, callers of decRef are responsible for that. That's why
something like:

Foo *f = new Foo();
Bar *b = Bar(f);

in my head f is deleted before being passed to Bar(). But

Foo *f = new Foo();
Bar *b = Bar(f);

makes sense, we know Bar() has taken its ref, so we can release ours. I
know with the current implementation both snippets are exactly the same, though.

> I've done a few changes making all the incRef/decRef private.
> Now the the pattern is:
>  * If you pass a Dict/Annot/Array to Object it will "take" it, that's most of 
> the times correct because if you do that is because you just new'ed the Dict/
> Annot/Array yourself
>  * If what you want to do is incRef, you should be passing Objects and not raw 
> Dict/Annot/Array
> Does that work for you?

I think the rule of thumb here should be that if an object releases a
reference, it should take one instead of relying on the caller to do
so. I also think that all Object constructors should be consistent, when
you create an Object passing a GooString, the string is copied, because
it's freed in the destructor, so the same logic should apply to
refcounted objects. I see however that it is unconvenient having to
decRef everytime we create an Object for a newly created refcounted

Foo *f = new Foo();
Object obj = new Object(f);

could be just

Object obj = new Object(new Foo());

But, we could achieve that using std:unique_ptr, for example and adding
a constructor receiving a && for that. Then we would just do:

Object obj = new Object(std::make_unique<Foo>());

then it's obvious when you are transfering the ownership or not. Or even
better if we use std::shared_ptr or whatever for refcounted objects and
get rid of incRef/decRef entirely.

In any case, I'll look at the new changes, because I haven't looked at
them yet :-P

> Cheers,
>   Albert
>> >  }

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: 194 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/poppler/attachments/20170513/e67e67c5/attachment.sig>

More information about the poppler mailing list