Re: [PATCH v3 3/5] msxml3: Implement ::get_attributes() for "xml" processing instruction node.

[Nikolay Sivov]

On 5/7/21 9:40 AM, Dmitry Timoshkov wrote:
> Nikolay Sivov  wrote:
> > On 4/27/21 12:27 PM, Dmitry Timoshkov wrote:
> > > - /* xmlSetProp/xmlSetNsProp accept only nodes of type XML_ELEMENT_NODE,
> > > -
> > > * so we have to create and assign attributes to a node by hand.
> > > -
> > > */
> > > -
> > > - if (version)
> > > - {
> > > -
> > >    attr = xmlSetNsProp(NULL, NULL, (const xmlChar *)"version", version);
> > > -
> > >    if (attr)
> > > -
> > >    {
> > > -
> > >        attr->doc = node->doc;
> > > -
> > >        set_prop(node, attr);
> > > -
> > >    }
> > > -
> > >    else hr = E_OUTOFMEMORY;
> > > - }
> >
> > This is abusing libxml2 tree structure, because 'properties' is expected
> > only for XML_ELEMENT_NODE nodes. I don't think we should break that
> > assumption.
>
> This code carefully followes what libxml2 does itself for storing properties,
> as far as I can see from the libxml2 source code this approach only leads to
> using libxml2 to free properties with the node. Why do you call this an "abuse"
> and think that's not acceptable? What would you suggest to use instead?

Because it's not meant for nodes other than elements. As far as I can
tell xmlFreeNode() won't free them for example. Similar to how <?xml ?>
is not a node in libxml2, its attributes are not properties in libxml2.