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();
f->decRef();
Bar *b = Bar(f);

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

Foo *f = new Foo();
Bar *b = Bar(f);
f->decRef();

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
object.

Foo *f = new Foo();
Object obj = new Object(f);
f->decRef();

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>