Re: [xml] Potential wrong usage of xmlIsID() in tree.c

[cazic gmx net]

Hi,

> Von: Rob Richards <rrichards ctindustries net>
> Datum: Fri, 24 Feb 2006 17:18:09 -0500
>
> Kasimier Buchcik wrote:

[...]

> > Some other internal functions come with this, but they might not be
> > needed; I just wanted to minimize the relevant code.
> >   
> A lot of the code can be elimintated using existing functionality, such 
> as what's the different between the xmlGetProp functions and just 
> xmlHasNsProp or xmlTreeNewPropRaw and xmlNewDocProp?

Yeah, right, this is all very explicit code not caring about existent
functionality; it needs to be merged with existing functionality.

> The one question I have regards the function signatures. I'm assuming 
> that the ones with leading comments are the ones to be exposed 
> (everything marked static right now). For the options parameter. Are 
> options going to be used often? If not, it might make sense to make them 
> part of the xmlDOMWrapCtxt structure.

I marked it optional, since we do not yet use the context in those
functions. But this is just an initial design, and it might end up being
mandatory at the end.

> Also, since the context is optional, it might make sense to pass it last 
> (the functions then are very close to the existing libxml2 functions.

I wanted the arg-list to be similiar to the other xmlDOMWrapXXX functions.

> I would also eleminate the doc parameter in most cases. This should be 
> able to be determined from the node being passed in (at least for these 

OK, for me.

> functions). Lastly, if the function is only returning a 0 or -1, would 
> it make more sense to just return the actual attribute (again similar to 
> the libxml2 function).
> i.e.:
> xmlAttrPtr
> xmlDOMWrapSetAttr(xmlNodePtr owner,
>           xmlNsPtr ns,
>           const xmlChar *name, const xmlChar *value,
>           xmlDOMWrapCtxtPtr ctxt)

Do we know how we will end up in the end? We'll lose flexibility for return
values.

> Moving the context paramter is not a big deal as I see it is already 
> passed first in other of the xmlDOM... functions. Just thought it would 
> make the mapping to the existing similar functions easier if possible.

It recurred to me that we need also a check for fixed attributes being tried
to be modified/replaced, in which case it would be nice to return the
corresponding DOM error NO_MODIFICATION_ALLOWED_ERR.
Although the INUSE_ATTRIBUTE_ERR can be handled on the wrapper's side, do
you think we should add that as well?

Regards,

Kasimier