XML Ranges

[DanRStevens]

I was looking into how to reduce coupling between the Configuration and Sprite classes with the TinyXML library. As part of that, I was looking at simplifying the loop code by using range-based for loops. It can be done with a bit of support code. I believe it might look something like this (compiles, but untested):

class EachChild {
public:
	class Iterator {
	public:
		Iterator(XmlNode* node) : node(node) {}

		XmlNode& operator*() { return *node; }
		void operator++() { node = node->nextSibling(); }
		bool operator!=(Iterator other) { return node != other.node; }
	private:
		XmlNode* node;
	};

	EachChild(XmlNode& parentNode) : parentNode(parentNode) {}
	Iterator begin() { return Iterator(parentNode.firstChild()); }
	Iterator end() { return Iterator(nullptr); }
private:
	XmlNode& parentNode;
};

Usage would then look like:

for (const auto& xmlNode : EachChild(*root))
{
	// ...
}

The two iterator cases I think we need to support are for child nodes, and for attributes.

I haven't yet given much thought to const iterators with the above.

---

Reference: How to make my custom type to work with “range-based for loops”?

Summary:
Iterator type, with support for: operator*, operator++, operator!=
Range type, with support for: begin(), end() (can be member methods, or free-standing with parameter)

As we likely want to support two types of iteration (children and attributes) using the same base node, we likely want custom range types for each case, with member begin() and end() on the range types.

[cugone]

What's the advantage of this over using a function that calls a callback inside a for loop?

void ForEachChildElement(const XMLElement& element, const std::string& childname /*= std::string{}*/, const std::function<void(const XMLElement&)>& callback /*= [](const XMLElement&) { / * DO NOTHING * / }*/) noexcept {
    auto childNameAsCStr = childname.empty() ? nullptr : childname.c_str();
    for(auto xml_iter = element.FirstChildElement(childNameAsCStr);
        xml_iter != nullptr;
        xml_iter = xml_iter->NextSiblingElement(childNameAsCStr))
    {
        callback(*xml_iter);
    }
}

void ForEachAttribute(const XMLElement& element, const std::function<void(const XMLAttribute&)>& callback /*= [](const XMLAttribute&) { / * DO NOTHING * / }*/) noexcept {
    for(auto attribute = element.FirstAttribute();
        attribute != nullptr;
        attribute = attribute->Next())
    {
        callback(*attribute);
    }
}

It seems to me you would end up writing way more code than if you just extract out the boilerplate repeated code into a simple function.

[DanRStevens]

Well, the main advantage is that you can use range-for loops. There's also an efficiency gain over using runtime callback, particularly std::function based ones, which can have a lot of overhead, and may even dynamically allocate memory in some cases.

You can improve the efficiency of the callback approach by using a template method with a function parameter. Having the called method known at compile time means the compiler can inline the called method. Passing a lambda to the method should then compile to about the same efficiency as the range-for loop. That imposes lambda syntax on the caller though.

So yeah, the code to support range-for syntax is a little larger, though efficiency is quite good, and the syntax benefits are nice. Either way though, the support code is packaged away in a library, so users don't need to worry about it.

Oh, and C++20 is getting some major additions in terms of Range support. Having these classes would allow us to make use of those additions for more complicated algorithms, such as filtering and transforming ranges.

[DanRStevens]

Reference: C++20 Ranges library.

[DanRStevens]

Some more good references:
LESSON #2: RANGE-BASED FOR
Enabling range-based for loops for custom types

---

TinyXML Documentation:
TinyXML1
TinyXML2

---

One thing to watch out for is const correctness. For ranges, generally there are two iterator types, one for the normal underlying type, and one for the const version of the underlying type. Note that the iterator itself cannot be const since it needs to increment across the range. The typical solution is to use a template class for the iterator, and vary the underlying type by either const or non-const.

I noticed a few TinyXML functions come in const and non-const pairs. That means there is some sense of const correctness built into the library, though it doesn't seem to be consistent or complete. That may hamper efforts to be fully const correct with a new range type.

---

I've noticed at least 3 sets of objects that might be iterated over:

• XMLNode - FirstChild/NextSibling
• XMLElement - FirstChildElement/NextSiblingElement
• XMLAttribute - FirstAttribute/Next

---

We could support both forward iterators and reverse iterators. For reverse iteration we would use:

• XMLNode - LastChild/PreviousSibling
• XMLElement - LastChildElement/PreviousSiblingElement

There does not appear to be reverse iteration support for XMLAttribute.

[cugone]

There does not appear to be reverse iteration support for XMLAttribute.

Can easily be added. I added UInt64 support and it was approved and merged into the library.

[DanRStevens]

Exactly what I was thinking. Submit some changes upstream for const correctness. Then maybe follow that up with range-for support. It probably makes the most sense to submit that work to the upstream project so everyone can benefit.

And yeah, I noticed you made that change to TinyXML.

[ldicker83]

Thinking further about this, the Dictionary class may be a good way to decouple NAS2D entirely from XML -- ultimately I'd like to deprecate and remove the exposed XML library functions and remove it from NAS2D ultimately requiring the end user to decide what storage format they want to use.