From 9e322aa7000a53325f83afd3a4f000444b447a90 Mon Sep 17 00:00:00 2001 From: Xeno Snow Fox Date: Sat, 12 Sep 2026 18:43:05 +1000 Subject: [PATCH 1/3] Use raw INode* for node children Replace std::shared_ptr with raw INode* in NodeContainer and TagNode. NodeContainer now owns child pointers and deletes them in its destructor; append() accepts INode* (with null-check and existing cycle detection preserved); children() returns a vector. operator<< overloads were updated to take INode* and TextNode instances are allocated with new. This change simplifies the ownership model by removing shared_ptr usage for XML node children. --- include/xtrpg/xml/node/NodeContainer.hpp | 48 +++++++++++++----------- include/xtrpg/xml/node/TagNode.hpp | 8 ++-- 2 files changed, 30 insertions(+), 26 deletions(-) diff --git a/include/xtrpg/xml/node/NodeContainer.hpp b/include/xtrpg/xml/node/NodeContainer.hpp index 599e276..dfe4398 100644 --- a/include/xtrpg/xml/node/NodeContainer.hpp +++ b/include/xtrpg/xml/node/NodeContainer.hpp @@ -18,35 +18,40 @@ class NodeContainer : public INode { explicit NodeContainer(NodeType type) : INode(type) {} - ~NodeContainer() = default; + ~NodeContainer() { + for (INode *child : _children) { + delete child; + } + this->_children.clear(); + }; /** * Appends a given child node. * * @throws std::invalid_argument if appending would create a cycle. */ - void append(std::shared_ptr child) { - if (child) { - // Check if this node is already an ancestor of child by walking up - // the parent chain. This is O(depth) instead of O(n). - for (NodeContainer *ancestor = child->getParent(); ancestor != nullptr; - ancestor = ancestor->getParent()) { - if (ancestor == this) { - throw std::invalid_argument("Cannot create a cycle in XML nodes"); - } + void append(INode *ptrChild) { + if (nullptr == ptrChild) { + return; + } + + // Check if this node is already an ancestor of child by walking up + // the parent chain. This is O(depth) instead of O(n). + for (NodeContainer *ancestor = ptrChild->getParent(); ancestor != nullptr; + ancestor = ancestor->getParent()) { + if (ancestor == this) { + throw std::invalid_argument("Cannot create a cycle in XML nodes"); } - // Set this node as the child's parent and append - child->setParent(this); - this->_children.push_back(std::move(child)); } + // Set this node as the child's parent and append + ptrChild->setParent(this); + this->_children.push_back(ptrChild); } /** * Returns a vector of child nodes. */ - const std::vector> &children() const { - return this->_children; - } + const std::vector &children() const { return this->_children; } /** * Checks whether this node has any child nodes. @@ -67,15 +72,14 @@ class NodeContainer : public INode { } private: - std::vector> _children; + std::vector _children; }; /** * Stream operator overload for easy serialization */ -inline NodeContainer &operator<<(NodeContainer &node, - std::shared_ptr child) { - node.append(std::move(child)); +inline NodeContainer &operator<<(NodeContainer &node, INode *child) { + node.append(child); return node; } @@ -85,8 +89,8 @@ inline NodeContainer &operator<<(NodeContainer &node, */ inline NodeContainer &operator<<(NodeContainer &node, const std::string &withText) { - auto textNode = std::make_shared(withText); - node.append(textNode); + TextNode *_ptrNode = new TextNode(withText); + node.append(_ptrNode); return node; } } // namespace xtrpg::xml::node \ No newline at end of file diff --git a/include/xtrpg/xml/node/TagNode.hpp b/include/xtrpg/xml/node/TagNode.hpp index d38d0ea..1e88fae 100644 --- a/include/xtrpg/xml/node/TagNode.hpp +++ b/include/xtrpg/xml/node/TagNode.hpp @@ -78,8 +78,8 @@ class TagNode : public ITagname, public IAttributes, public NodeContainer { /** * Stream operator overload for easy serialization */ -inline TagNode &operator<<(TagNode &node, std::shared_ptr child) { - node.append(std::move(child)); +inline TagNode &operator<<(TagNode &node, INode *ptrNode) { + node.append(std::move(ptrNode)); return node; } @@ -88,8 +88,8 @@ inline TagNode &operator<<(TagNode &node, std::shared_ptr child) { * child containing the provided text. */ inline TagNode &operator<<(TagNode &node, const std::string &withText) { - auto textNode = std::make_shared(withText); - node.append(textNode); + TextNode *_ptrNode = new TextNode(withText); + node.append(_ptrNode); return node; } } // namespace xtrpg::xml::node \ No newline at end of file From fa3dab1039a1836ee300c0f47a0be01f8af81aec Mon Sep 17 00:00:00 2001 From: Xeno Snow Fox Date: Sat, 12 Sep 2026 19:03:04 +1000 Subject: [PATCH 2/3] Make XML nodes move-only; improve append safety Delete copy ctor/assignment for DeclarationNode, TagNode and NodeContainer to enforce move-only ownership of child nodes; keep/default move semantics on NodeContainer. Wrap operator<< overloads for TagNode and NodeContainer in try/catch to delete the temporary TextNode on failure and prevent leaks. Changes improve ownership safety (avoid shallow copies) and exception-safety when appending text nodes. --- include/xtrpg/xml/node/DeclarationNode.hpp | 9 +++++---- include/xtrpg/xml/node/NodeContainer.hpp | 15 ++++++++++++++- include/xtrpg/xml/node/TagNode.hpp | 17 ++++++++++++----- 3 files changed, 31 insertions(+), 10 deletions(-) diff --git a/include/xtrpg/xml/node/DeclarationNode.hpp b/include/xtrpg/xml/node/DeclarationNode.hpp index 23038d9..14d4069 100644 --- a/include/xtrpg/xml/node/DeclarationNode.hpp +++ b/include/xtrpg/xml/node/DeclarationNode.hpp @@ -27,9 +27,9 @@ class DeclarationNode : public INode, public ITagname, public IAttributes { } /** - * Explicitly defaulted copy constructor. + * Deleted copy constructor - move-only semantics for consistency and safety. */ - DeclarationNode(const DeclarationNode &) = default; + DeclarationNode(const DeclarationNode &) = delete; /** * Explicitly defaulted move constructor. @@ -37,9 +37,10 @@ class DeclarationNode : public INode, public ITagname, public IAttributes { DeclarationNode(DeclarationNode &&) = default; /** - * Explicitly defaulted copy assignment operator. + * Deleted copy assignment operator - move-only semantics for consistency and + * safety. */ - DeclarationNode &operator=(const DeclarationNode &) = default; + DeclarationNode &operator=(const DeclarationNode &) = delete; /** * Explicitly defaulted move assignment operator. diff --git a/include/xtrpg/xml/node/NodeContainer.hpp b/include/xtrpg/xml/node/NodeContainer.hpp index dfe4398..9e4a885 100644 --- a/include/xtrpg/xml/node/NodeContainer.hpp +++ b/include/xtrpg/xml/node/NodeContainer.hpp @@ -18,6 +18,14 @@ class NodeContainer : public INode { explicit NodeContainer(NodeType type) : INode(type) {} + // Delete copy semantics - ownership transfer is explicit via move + NodeContainer(const NodeContainer &) = delete; + NodeContainer &operator=(const NodeContainer &) = delete; + + // Keep move semantics to allow ownership transfer + NodeContainer(NodeContainer &&) = default; + NodeContainer &operator=(NodeContainer &&) = default; + ~NodeContainer() { for (INode *child : _children) { delete child; @@ -90,7 +98,12 @@ inline NodeContainer &operator<<(NodeContainer &node, INode *child) { inline NodeContainer &operator<<(NodeContainer &node, const std::string &withText) { TextNode *_ptrNode = new TextNode(withText); - node.append(_ptrNode); + try { + node.append(_ptrNode); + } catch (...) { + delete _ptrNode; + throw; + } return node; } } // namespace xtrpg::xml::node \ No newline at end of file diff --git a/include/xtrpg/xml/node/TagNode.hpp b/include/xtrpg/xml/node/TagNode.hpp index 1e88fae..d040f60 100644 --- a/include/xtrpg/xml/node/TagNode.hpp +++ b/include/xtrpg/xml/node/TagNode.hpp @@ -31,9 +31,10 @@ class TagNode : public ITagname, public IAttributes, public NodeContainer { : ITagname(name), IAttributes(), NodeContainer(NodeType::TAG) {} /** - * Explicitly defaulted copy constructor. + * Deleted copy constructor - prevents accidental shallow copies of child + * nodes. Use move semantics for explicit ownership transfer. */ - TagNode(const TagNode &) = default; + TagNode(const TagNode &) = delete; /** * Explicitly defaulted move constructor. @@ -41,9 +42,10 @@ class TagNode : public ITagname, public IAttributes, public NodeContainer { TagNode(TagNode &&) = default; /** - * Explicitly defaulted copy assignment operator. + * Deleted copy assignment operator - prevents accidental shallow copies of + * child nodes. Use move semantics for explicit ownership transfer. */ - TagNode &operator=(const TagNode &) = default; + TagNode &operator=(const TagNode &) = delete; /** * Explicitly defaulted move assignment operator. @@ -89,7 +91,12 @@ inline TagNode &operator<<(TagNode &node, INode *ptrNode) { */ inline TagNode &operator<<(TagNode &node, const std::string &withText) { TextNode *_ptrNode = new TextNode(withText); - node.append(_ptrNode); + try { + node.append(_ptrNode); + } catch (...) { + delete _ptrNode; + throw; + } return node; } } // namespace xtrpg::xml::node \ No newline at end of file From db68184193020ca6d4713a472f8f6a414b7936f4 Mon Sep 17 00:00:00 2001 From: Xeno Snow Fox Date: Sat, 12 Sep 2026 19:15:22 +1000 Subject: [PATCH 3/3] Add append overloads for Text and Tag nodes Introduce convenience append overloads and improve exception safety. - include/xtrpg/xml/node/NodeContainer.hpp: add append(const std::string&) to create and append a TextNode (with RAII-style cleanup on error). - include/xtrpg/xml/node/TagNode.hpp: add template append(tagname, consumer) to construct a TagNode, allow caller configuration via a consumer, and append it safely (cleanup on exception). Includes example usage. - Qualify calls in operator<< to NodeContainer::append to avoid overload ambiguity. Enables fluent, nested XML construction and better error safety. --- include/xtrpg/xml/node/NodeContainer.hpp | 20 ++++++++++++ include/xtrpg/xml/node/TagNode.hpp | 39 ++++++++++++++++++++++-- 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/include/xtrpg/xml/node/NodeContainer.hpp b/include/xtrpg/xml/node/NodeContainer.hpp index 9e4a885..0ddbefe 100644 --- a/include/xtrpg/xml/node/NodeContainer.hpp +++ b/include/xtrpg/xml/node/NodeContainer.hpp @@ -56,6 +56,26 @@ class NodeContainer : public INode { this->_children.push_back(ptrChild); } + /** + * Appends a new TextNode containing the provided string to this container. + * Exception-safe: if append fails, the TextNode is cleaned up before + * re-throwing the exception. + * + * @param withText the text content for the new TextNode + * @throws std::invalid_argument if the text contains invalid XML characters + * or if appending would create a cycle (though cycles are not + * possible with newly created TextNodes). + */ + void append(const std::string &withText) { + TextNode *_ptrNode = new TextNode(withText); + try { + this->append(_ptrNode); + } catch (...) { + delete _ptrNode; + throw; + } + } + /** * Returns a vector of child nodes. */ diff --git a/include/xtrpg/xml/node/TagNode.hpp b/include/xtrpg/xml/node/TagNode.hpp index d040f60..8728f0f 100644 --- a/include/xtrpg/xml/node/TagNode.hpp +++ b/include/xtrpg/xml/node/TagNode.hpp @@ -57,6 +57,41 @@ class TagNode : public ITagname, public IAttributes, public NodeContainer { */ const std::string_view name() const { return this->getTagname(); } + /** + * Appends a new TagNode with the provided tag name to this container. + * The consumer function is called with the new TagNode to allow configuration + * before it is appended. This pattern enables fluent, nested construction of + * XML trees. + * + * Exception-safe: if the consumer throws or append fails, the TagNode is + * cleaned up before re-throwing the exception. + * + * @param tagname the name for the new TagNode + * @param consumer a callable that accepts a TagNode& for configuration + * @throws std::invalid_argument if the tag name is invalid or if the + * consumer throws an exception + * + * Example usage: + * @code + * container.append("error", [](TagNode& error) { + * error.append("message", [](TagNode& msg) { + * msg.append("Something went wrong"); + * }); + * }); + * @endcode + */ + template + void append(const std::string &tagname, Consumer &&consumer) { + TagNode *tagNode = new TagNode(tagname); + try { + consumer(*tagNode); + this->NodeContainer::append(tagNode); + } catch (...) { + delete tagNode; + throw; + } + } + /** * Serializes the node into an XML formatted string. */ @@ -81,7 +116,7 @@ class TagNode : public ITagname, public IAttributes, public NodeContainer { * Stream operator overload for easy serialization */ inline TagNode &operator<<(TagNode &node, INode *ptrNode) { - node.append(std::move(ptrNode)); + node.NodeContainer::append(std::move(ptrNode)); return node; } @@ -92,7 +127,7 @@ inline TagNode &operator<<(TagNode &node, INode *ptrNode) { inline TagNode &operator<<(TagNode &node, const std::string &withText) { TextNode *_ptrNode = new TextNode(withText); try { - node.append(_ptrNode); + node.NodeContainer::append(_ptrNode); } catch (...) { delete _ptrNode; throw;