Fri May 12 13:31:55 UTC 2017

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.

>  }
>  

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