Extending pugixml (xml_node) functionality for another library

[aral-matrix]

While working on the OpenXLSX library, I implemented

• whitespace support and accordingly activated it within pugixml
• namespace support for which I had to wrap pugixml functions referring to a node name (or returning it)

I found myself in need of implementing some additional functionality to find next child / sibling nodes based on node type (defaulting to pugi::node_element), and of course wrapper functions for anything referring to a node by name.

My current implementation derives a class from pugi::xml_node (and one from pugi::xml_document) to implement the wrapper functions and change the return type of the base class functions to the derived class when an xml_node is returned.

I am absolutely not happy with my implementation, as it exposes the pugixml headers through library headers to the user of the library (and accordingly can cause conflicts with a local pugixml installation). Accordingly I am trying to improve it.

Now in order to prepare packaging for linux distributions, I started looking into doing a pImpl idiom extension of pugixml that can work with an installed pugixml library.
However, that turns out to be

1. more complicated than I thought (iterators and iterator ranges behind a pImpl are really complicated)
2. will likely impact performance significantly, both due to the frequent access to pugi classes while iterating, as well as requiring additional conversions between pugi internal data types and data types in the extended interface - since forward declarations on e.g. enums used by pugixml are not possible per C++ standard (afaik)

So my question: is there a better way than pImpl to "augment" the pugixml library? And if so - how would you (@zeux) go about that?

My assumption is that - even if pugixml is extended by the functions that I need for OpenXLSX - it will take months if not years to make its way into linux distro packages, so that I wouldn't gain much by depending on a very new pugixml library version.

[zeux]

I don't really understand why there would be conflicts with pugixml that's externally packaged if you inherit from pugixml classes. It would seem like things should work just fine unless you are changing pugixml header/source file; if you aren't, I don't think pimpl will help here.

But regardless, the only two reasonably clean options for class extension I know of are either inheritance, which you tried, or external free functions (which you can define in your own header, and put in either the common namespace in your application or even in pugi:: namespace to take advantage of argument dependent lookup). So you'd have something like child_ns(node, name, "xlsx") instead of node.child_ns(name, "xlsx").

I always prefer the latter, as it's cleaner and doesn't suffer from accidental object slicing that may change behavior (e.g. using an inherited API on xml_node that wasn't shadowed would return pugi::xml_node, and calling child on that would use the original method) - because every call is explicit. It's also way simpler as you don't need to duplicate functionality just for the sake of forwarding the calls.

[aral-matrix]

Thank you for your considerations & insight.

This may be my ignorance speaking, but I wasn't sure how external free functions would be able to benefit from the fast access to the xml_node internal member variables. That is why I went with inheritance.

I finally had the right idea & was able to work around my problem by not trying to hide pugixml itself, but rather hide my derived classes from the public library headers.

https://github.com/troldal/OpenXLSX/blob/development-aral/OpenXLSX/headers/XLXmlParser.hpp

For that to work I had to make a separate header with forward declarations and because of this logic I had to replace a few remaining XMLNode member variables with pointers, and I still have to perform a code review to replace all defaulted constructors and assignment operators for classes that use such a pointer - because unfortunately those do not trigger a compiler error unless the class is actually instantiated in the application being compiled.

What I tried the other day was to then compile against pugixml release 1.15 from this repo, and I realized I have another problem for packaging OpenXLSX on debian:

So far, I was using pugixml in header-only mode, which allowed me to "see" in my XLXmlParser module all contents of the pugixml code. But when I try and switch to the static (or dynamic) library, I only see the header file, and that leaves me with a few structures I no longer have access to. I didn't yet analyze this in full, but what I already know I am missing is:

xml_node_struct
and this internal #define from within pugi::impl:
# define PUGI_IMPL_NODETYPE(n) static_cast<pugi::xml_node_type>((n)->header & pugi::impl::xml_memory_page_type_mask)

For use like so:

    XMLNode XMLNode::first_child_of_type(pugi::xml_node_type type_) const
    {
        if (_root) {
            XMLNode x = first_child();
            XMLNode l = last_child();
            while (x != l && x.type() != type_) x = x.next_sibling();
            if (x.type() == type_)
                return XMLNode(x);
        }
        return XMLNode();    // if no node matching type_ was found: return an empty node
    }

Any chance you could move those types (the struct, potentially others, and the PUGI_IMPL_NODETYPE define) to a separate header file, to be included from pugixml.cpp, but also available to applications like the one I am working on, to allow performant extension of library classes?

[zeux]

Ah, if you are trying to use pugixml internals then yes, you'd be effectively requiring a header-only build. The internal structures and macros are, well, internal; they are not designed to be usable externally. If you use a header-only build, you do not need them for performance due to inlining; if you do not use a header-only build, exposing them is fraught with peril because the structure and implementation here depends on the exact version of the library, so using these internals is not ABI safe: existing binaries may break and require recompilation across minor versions of the library.

[aral-matrix]

(apologies for the long reply in advance)

Well - my issue is that - as you can maybe see from my exemplary function above - I do not see a performant way to provide this functionality without access to the internals - and packaging for debian (e.g.) requires that external dependencies should not be shipped as part of depending package, but rather be correctly identified.
From what I can see, the header-only version of pugixml is not available when libpugixml is installed as a OS package (I do not see the pugixml.cpp file being copied to the host OS).

So my friendly request for consideration by you is what I mentioned above:
Would you be willing to move part of the pugi::impl namespace internal structure definitions outside into a second header file, e.g. pugixml-internals.hpp - that would then be included only by pugixml.cpp, so nothing changes on your library API, but the pugixml-internals.hpp would be installed to the target system and could be available to people like me, for library extensions?

The goal would be that - as a first step - OpenXLSX, if compiled against a given pugixml version - will "inherit" any changes you make to those definitions, and would not break.

From what I see at first glance, what would be needed (for my current use case, more might be useful to other people) is:

// (global namespace)
#define PUGI_IMPL_STATIC_ASSERT(cond) { static const char condition_failed[(cond) ? 1 : -1] = {0}; (void)condition_failed[0]; }

// (pugi::impl namespace)
	static const uintptr_t xml_memory_page_type_mask = 15;

as used in the also needed macro:

// (pugi::impl namespace)
 #   define PUGI_IMPL_NODETYPE(n) static_cast<pugi::xml_node_type>((n)->header & pugi::impl::xml_memory_page_type_mask)

and

        // pugi::impl namespace
        struct xml_memory_page { /**/ }

        // pugi namespace
        struct xml_attribute_struct { /**/ }
	struct xml_node_struct { /**/ }

In the long run, would you be interested in adding to pugixml

• child / sibling "by node type" functions?
• namespace support(*)?

(*) My current namespace support implementation stores with the node an int offset to the node name start after a potential namespace separator ':' and - upon access to nodes by name - discards namespaces from the passed-in node name unless force_ns == true. Created nodes will inherit the namespace from their parent node. With force_ns set, nodes will be 1) accessed by the full provided name (including XML namespace), and 2) will be created using the provided name including a namespace, so that you can achieve something like:

<xs:row>
   <xc:cell>
      <xc:text />
   </xc:cell>
<xs:row>

by

1. creating a node "xs:row" with force_ns = true
2. inserting a child "xc:cell" with force_ns = true
3. inserting a child "text" with force_ns = false (default)

[zeux]

As I noted in my previous comments, exposing internal details is a problem for binary (and interface) compatibility, so no, I'm not willing to do that.

I've looked at OpenXSLSX source briefly and I don't fully understand why "by type" functionality is particularly interesting; a few loops of that nature seem like they could just be replaced by the usual iterations without changing the semantics. When that's not the case, I'm very skeptical that a single call to .type() in the loop body per iteration is material for performance on top of everything else that happens in the code. In principle I'm not opposed to some sort of addition there if it's minimal, but I don't fully understand the use case here and why it needs to be in the library, as it's the first time I hear of this request.

Namespace support is potentially interesting but it must be lightweight and layered on top of the existing code in some way. #367 has some commentary but I haven't looked at this in years so it may be out of date with my current thinking. Certainly I would not want append_child("foo") to prepend the namespace qualifier automatically, or to store anything namespace related in the node structure or object.

[aral-matrix]

Well - it didn't hurt to ask.
I just had a first attempt at what I was requesting, which can be found here. To be fair, I have no idea why it works without forward declarations for structs that are internal to pugixml.cpp, but I can compile OpenXLSX against the pugixml library installed (in not-header-only mode) via cmake.

The "by type" functionality is mostly a generalized implementation for what I actually need: "get element node" or "get whitespace node". It became interesting when I wanted to support whitespaces so that pretty-formatted input XLSX contents are not mutilated upon saving - this was crucial for debugging OOXML standard support (i.e. finding out what I had messed up in OpenXLSX vs. how LibreOffice or Excel were storing files).

Regarding performance, I would have to do some benchmarking - but for Excel spreadsheets, the XML can easily have many millions of elements, and finding cells by index (random access) requires iterating over a lot of XML, so the additional call to type() would potentially happen a few million times.

Certainly I would not want append_child("foo") to prepend the namespace qualifier automatically,

Understandable - but if the automatic prepend could be a configuration option, that would be great - I made OpenXLSX basically namespace agnostic, the support for namespaces is then under the hood in XLXmlParser / the xml_node derived class. It would not be amusing to have to specify a node namespace in every single call.

or to store anything namespace related in the node structure or object.

Also, very understandable considering you have focused on performance quite successfully. I guess namespace support could dynamically extract the non-namespaced name in a dedicated function, e.g. xml_node::name_no_ns() and allow access to namespaced nodes by their name (without the namespace) in a dedicated function.

For the time being, I will have to analyze the performance impact of implementing the child / sibling "of type" methods without access to pugi internals, and to see if I can rewrite everything so that it is compatible with the form in which pugixml is shipped as a library.

Thank you for your time!