Carlos Garcia Campos <[email protected]> writes: > 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
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
signature.asc
Description: PGP signature
_______________________________________________ poppler mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/poppler
