Re: [pdf-devel] Patch for FS#56 (second try)

[gerel]

 > Date: Wed, 23 Jul 2008 01:54:45 +0200
 > From: address@hidden
 > 
 >     > Would be good to explain there what the function does if the parameter
 >     > is NULL.
 > 
 >    Sure.
 > 
 > Ok. Please add the complete description to your patch.

Ok.

 > 
 >     >    +  if (pdf_hash_add (props_hash, "isHidden", (void *) 
 > is_hidden,NULL) !=
 >     >    +      PDF_OK)
 >     >    +    {
 >     >    +      return PDF_ERROR;
 >     >    +    }
 >     > 
 >     > Will the memory used by 'is_hidden' be released when the hash is
 >     > destroyed being the last parameter NULL? As far as I understand in
 >     > this case the dispose function defined for the hash will be applied.
 > 
 >    Right, we have two choices,
 > 
 >    1. we pass each function call the element dispose function.
 > 
 > I would prefer this one.
 > 

Ok.

 >    2. we give it at creation time, i.e. pdf_hash_create(). But now
 >    that I see pdf_fsys_item_props_to_hash() doesn't create the hash
 >    table, which should if we want to do this.
 > 
 > I would remove the possibility to use a table-wide memory disposal
 > function. Hashes tend to hold heterogeous values. That is certainly
 > our case where we will use hashes to implement the PDF dictionaries.
 > 

I agree, it makes the API easier to use.


 >     >  newelt->key = key;
 >     >  ...
 >     >  gl_sortedlist_add ((gl_list_t)table.keys, key_compare, key);
 >     > 
 >     > Is 'gl_sortedlist_add' making a copy of 'key'?
 > 
 >    No, it is not. Also, remember that if the key is allocated from the
 >    heap you could give pdf_hash_create() the key dispose function if
 >    you need to.
 > 
 > The keys are always raw strings, isnt it? If it is the case I would
 > say to let the hash implementation to make a copy of it and manage its
 > memory. 

Yes they're raw strings, but I chose to let the caller decide this since when
using statically allocated strings (as in many cases) we would be duplicating
strings for nothing. I'm not a PDF guru but I guess _many_ of the string keys
we're going to use are predefined in the standard. So, I don't agree on this
modification.

cheers

-gerel