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

[Daniel Veillard <veillard redhat com>]

On Fri, Feb 03, 2006 at 05:24:30PM +0100, Kasimier Buchcik wrote:
> Hi,
>
> On Fri, 2006-02-03 at 11:13 -0500, Daniel Veillard wrote:
> > On Fri, Feb 03, 2006 at 03:52:46PM +0100, Kasimier Buchcik wrote:
> > > Hi,
> > >
> > > We have the following code in tree.c, xmlCopyPropInternal():
> > >
> > > if ((target!= NULL) && (cur!= NULL) &&
> > >   (target->doc != NULL) && (cur->doc != NULL) &&
> > >   (cur->doc->ids != NULL) && (cur->parent != NULL)) {
> > >   if (xmlIsID(cur->doc, cur->parent, cur)) {
> > >   [... add the id to the target doc ...]
> > > }
> > >
> > > This queries if the current attr is an ID in the _source_ doc via
> > > xmlIsID().
> > >
> > > I think this should check if the newly created attribute is an ID in
> > > the _destination_ doc.
> >
> >   Hum, true, sounds there is somthing fishy there, though the itent is
> > to keep the IDness property, i.e. if you used #foo in th old doc to point
> > to that node, well #foo will still point to it in the new one.
>
> Keep the IDness even if the target doc has a different or even no DTD?
> If the attribute is an xml:id then yes, but otherwise I would say no.

  yeah, I know. But I expect if we change this we will break XInclude
because it uses that routine, and it carries IDness forward independantly
of the result DTD. So if you change this, please adjust how XInclude 
uses it to make sure XInclutde ain't broken as a result.

> > > Just an observation: Currently, for branch-copy operations a DTD-lookup
> > > is performed (together with a QName building string operation) for every
> > > attribute (except for xml:id attributes).
> > >
> > > There is more code in tree.c using xmlIsID() and I think some of
> > > the calls should be substituted for the XML_ATTRIBUTE_ID-test.
> > > E.g. in xmlSetProp(), xmlIsID() is called for an existent
> > > attribute if it is about to be replaced by a new one.
> > >
> > > Doesn't xmlIsID() have to be called _only_ if a newly created
> > > attribute is added to the doc's tree? If it's already insideo
> >
> >    Hum, it just test, and should not have side effect so I don't see why
> > we should be more restrictive in its use, plus it's part of the public API...
> >
> > > the doc's tree, then XML_ATTRIBUTE_ID should be
> > > aready set - or am I missing something here?
> >
> >   unclear,
>
> Sorry, I was referring to the attr->atype == XML_ATTRIBUTE_ID check.
> This marker is set on the attribute in xmlAddID(). We wouldn't have to
> query through xmlIsID() if the marker is used to test IDness of attrs
> already added to the doc's tree.

  hum, right, that should work. I think thisd came from the fact that 
atype was added to xmlAttr but used to be only on xmlAttributes i.e. the
declarations in the DTd not duplicated in the instances.

Daniel

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