Sat May 13 07:35:45 UTC 2017

Carlos Garcia Campos <carlosgc at gnome.org> writes:

> 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

Ok, I see now that only Object is allowed to inc/decRef. Object is the
only one who can own refcounted objects, and always adopt in the
constructors. That works for me :-)

>> Cheers,
>>   Albert
>>
>>> 
>>> >  }
>>
>>
>
> -- 
> Carlos Garcia Campos
> PGP key: http://pgp.mit.edu:11371/pks/lookup?op=get&search=0x523E6462

-- 
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/7b5b8362/attachment.sig>