Re: [xml] function consolidation

[Daniel Veillard <veillard redhat com>]

On Fri, Mar 25, 2005 at 04:24:49PM -0500, Rob Richards wrote:
> Daniel Veillard wrote:
>
> > it must free the string before returning since the caller won't free it
> > clearly this was the previous behaviour.
> Hmm, I'm not too sure about that.
>
> The previous behavior (using testapi still) created and appended the 
> attribute on the PI node, so the call:
> ret_val = xmlNewNsPropEatName(node, ns, name, (const xmlChar *)value);
> would return an attribute node.
>
> The name was freed by the next line:
> desret_xmlAttrPtr(ret_val);
>
> Now, however, ret_val is NULL since its an invalid node to set the 
> attribute on and no attribute created..
> It now works the same way as xmlNewNodeEatName as in that function if 
> NULL is returned, the caller has to free the name. (NULL only returned 
> if xmlMalloc failed there).
>
> Your call on behavior here but wanted to make sure I was clear on what 
> is happening before changing it.

  I still think we should xmlFree() the name parameter. The underlying reason
is that it's an API refinement and I prefer an immediate crash immediately
identified in case someone expected a different behaviour than a slow memory
leak which may only be tracked painfully months later.
  The function eats the name that's expected I think.
  The case of xmlNewNodeEatName the only error is when the program runs out
of memory, then it will generate a leak, I will fix that, it's broken.

  We should really have this discussion on-list. Even if posting again the
patch was looking like a duplicate we should stick to the list, that's why
I'm adding it again. Someone may disagree with me for a good reason, and should
have a chance to object, so Cc'ed to the list again.

Daniel

-- 
Daniel Veillard      | Red Hat Desktop team http://redhat.com/
veillard redhat com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/