Albert Astals Cid <[email protected]> writes: > El divendres, 12 de maig de 2017, a les 15:31:55 CEST, Carlos Garcia Campos > va > escriure: >> Albert Astals Cid <[email protected]> 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
signature.asc
Description: PGP signature
_______________________________________________ poppler mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/poppler
