I'm attaching a patch set that implement the suggested approach Regards, Alessandro
From 069453201618f6d1768fa52fe149e85bf21b1227 Mon Sep 17 00:00:00 2001
From: Alessandro Pignotti <a pignotti sssup it>
Date: Fri, 5 Nov 2010 04:59:32 +0100
Subject: [PATCH 1/4] Do not register node handlers and move the code to create libxml++ instances inside Document
---
libxml++/document.cc | 50 +++-----------------------------------------------
libxml++/document.h | 2 ++
2 files changed, 5 insertions(+), 47 deletions(-)
diff --git a/libxml++/document.cc b/libxml++/document.cc
index 108b2e3..321608c 100644
--- a/libxml++/document.cc
+++ b/libxml++/document.cc
@@ -26,14 +26,12 @@
#include <iostream>
-namespace
+namespace xmlpp
{
-//Called by libxml whenever it constructs something,
-//such as a node or attribute.
-//This allows us to create a C++ instance for every C instance.
-void on_libxml_construct(xmlNode* node)
+void Document::create_wrapper(xmlNode* node)
{
+ assert(node->_private==NULL);
switch (node->type)
{
case XML_ELEMENT_NODE:
@@ -97,51 +95,9 @@ void on_libxml_construct(xmlNode* node)
}
}
-//Called by libxml whenever it destroys something
-//such as a node or attribute.
-//This allows us to delete the C++ instance for the C instance, if any.
-void on_libxml_destruct(xmlNode* node)
-{
- bool bPrivateDeleted = false;
- if (node->type == XML_DTD_NODE)
- {
- xmlpp::Dtd* cppDtd = static_cast<xmlpp::Dtd*>(node->_private);
- if(cppDtd)
- {
- delete cppDtd;
- bPrivateDeleted = true;
- }
- }
- else if (node->type == XML_DOCUMENT_NODE)
- // do nothing. See on_libxml_construct for an explanation
- ;
- else
- {
- xmlpp::Node* cppNode = static_cast<xmlpp::Node*>(node->_private);
- if(cppNode)
- {
- delete cppNode;
- bPrivateDeleted = true;
- }
- }
-
- //This probably isn't necessary:
- if(bPrivateDeleted)
- node->_private = 0;
-}
-
-} //anonymous namespace
-
-namespace xmlpp
-{
-
Document::Init::Init()
{
xmlInitParser(); //Not always necessary, but necessary for thread safety.
- xmlRegisterNodeDefault(on_libxml_construct);
- xmlDeregisterNodeDefault(on_libxml_destruct);
- xmlThrDefRegisterNodeDefault(on_libxml_construct);
- xmlThrDefDeregisterNodeDefault(on_libxml_destruct);
}
Document::Init::~Init()
diff --git a/libxml++/document.h b/libxml++/document.h
index c30068c..b0e0066 100644
--- a/libxml++/document.h
+++ b/libxml++/document.h
@@ -170,6 +170,8 @@ public:
///Access the underlying libxml implementation.
const _xmlDoc* cobj() const;
+ ///Construct the right C++ instances for a given element
+ static void create_wrapper(_xmlNode* node);
protected:
/** Retrieve an Entity.
* The entity can be from an external subset or internally declared.
--
1.7.2.3
From e368ea2756acba46c258e1393fe1abced673a20a Mon Sep 17 00:00:00 2001
From: Alessandro Pignotti <a pignotti sssup it>
Date: Fri, 5 Nov 2010 05:06:50 +0100
Subject: [PATCH 2/4] Use create_wrapper to allocate libxml++ classes on demand
---
libxml++/document.cc | 6 +++
libxml++/nodes/element.cc | 31 ++++++++++++++++-
libxml++/nodes/node.cc | 64 ++++++++++++++++++++++++----------
libxml++/parsers/textreader.cc | 14 ++++++++
libxml++/validators/dtdvalidator.cc | 5 +++
5 files changed, 99 insertions(+), 21 deletions(-)
diff --git a/libxml++/document.cc b/libxml++/document.cc
index 321608c..7860ba4 100644
--- a/libxml++/document.cc
+++ b/libxml++/document.cc
@@ -164,7 +164,11 @@ Element* Document::get_root_node() const
if(root == 0)
return 0;
else
+ {
+ if(root->_private==NULL)
+ create_wrapper(root);
return reinterpret_cast<Element*>(root->_private);
+ }
}
Element* Document::create_root_node(const Glib::ustring& name,
@@ -218,6 +222,8 @@ CommentNode* Document::add_comment(const Glib::ustring& content)
// Use the result, because node can be freed when merging text nodes:
node = xmlAddChild( (xmlNode*)impl_, node);
+ if(node->_private==NULL)
+ create_wrapper(node);
return static_cast<CommentNode*>(node->_private);
}
diff --git a/libxml++/nodes/element.cc b/libxml++/nodes/element.cc
index 69604f5..bb86838 100644
--- a/libxml++/nodes/element.cc
+++ b/libxml++/nodes/element.cc
@@ -7,6 +7,7 @@
#include <libxml++/nodes/element.h>
#include <libxml++/nodes/textnode.h>
#include <libxml++/exceptions/internal_error.h>
+#include <libxml++/document.h>
#include <libxml/tree.h>
@@ -25,6 +26,8 @@ Element::AttributeList Element::get_attributes()
AttributeList attributes;
for(xmlAttr* attr = cobj()->properties; attr; attr = attr->next)
{
+ if(attr->_private==NULL)
+ Document::create_wrapper(reinterpret_cast<xmlNode*>(attr));
attributes.push_back(reinterpret_cast<Attribute*>(attr->_private));
}
@@ -44,6 +47,8 @@ Attribute* Element::get_attribute(const Glib::ustring& name,
xmlAttr* attr = xmlHasProp(const_cast<xmlNode*>(cobj()), (const xmlChar*)name.c_str());
if( attr )
{
+ if(attr->_private==NULL)
+ Document::create_wrapper(reinterpret_cast<xmlNode*>(attr));
return reinterpret_cast<Attribute*>(attr->_private);
}
}
@@ -54,6 +59,8 @@ Attribute* Element::get_attribute(const Glib::ustring& name,
(const xmlChar*)ns_uri.c_str());
if( attr )
{
+ if(attr->_private==NULL)
+ Document::create_wrapper(reinterpret_cast<xmlNode*>(attr));
return reinterpret_cast<Attribute*>(attr->_private);
}
}
@@ -95,7 +102,11 @@ Attribute* Element::set_attribute(const Glib::ustring& name, const Glib::ustring
}
if(attr)
+ {
+ if(attr->_private==NULL)
+ Document::create_wrapper(reinterpret_cast<xmlNode*>(attr));
return reinterpret_cast<Attribute*>(attr->_private);
+ }
else
return 0;
}
@@ -117,7 +128,11 @@ const TextNode* Element::get_child_text() const
// FIXME: return only the first content node
for(xmlNode* child = cobj()->children; child; child = child->next)
if(child->type == XML_TEXT_NODE)
- return static_cast<TextNode*>(child->_private);
+ {
+ if(child->_private==NULL)
+ Document::create_wrapper(child);
+ return static_cast<TextNode*>(child->_private);
+ }
return 0;
}
@@ -128,7 +143,11 @@ TextNode* Element::get_child_text()
// What should we do instead? Update the documentation if we change this. murrayc.
for(xmlNode* child = cobj()->children; child; child = child->next)
if(child->type == XML_TEXT_NODE)
- return static_cast<TextNode*>(child->_private);
+ {
+ if(child->_private==NULL)
+ Document::create_wrapper(child);
+ return static_cast<TextNode*>(child->_private);
+ }
return 0;
}
@@ -151,6 +170,8 @@ TextNode* Element::add_child_text(const Glib::ustring& content)
// Use the result, because node can be freed when merging text nodes:
node = xmlAddChild(cobj(), node);
+ if(node->_private==NULL)
+ Document::create_wrapper(node);
return static_cast<TextNode*>(node->_private);
}
return 0;
@@ -168,6 +189,8 @@ TextNode* Element::add_child_text(xmlpp::Node* previous_sibling, const Glib::ust
// Use the result, because node can be freed when merging text nodes:
node = xmlAddNextSibling(previous_sibling->cobj(), node);
+ if(node->_private==NULL)
+ Document::create_wrapper(node);
return static_cast<TextNode*>(node->_private);
}
return 0;
@@ -185,6 +208,8 @@ TextNode* Element::add_child_text_before(xmlpp::Node* next_sibling, const Glib::
// Use the result, because node can be freed when merging text nodes:
node = xmlAddPrevSibling(next_sibling->cobj(), node);
+ if(node->_private==NULL)
+ Document::create_wrapper(node);
return static_cast<TextNode*>(node->_private);
}
return 0;
@@ -226,6 +251,8 @@ CommentNode* Element::add_child_comment(const Glib::ustring& content)
// Use the result, because node can be freed when merging text nodes:
node = xmlAddChild(cobj(), node);
+ if(node->_private==NULL)
+ Document::create_wrapper(node);
return static_cast<CommentNode*>(node->_private);
}
diff --git a/libxml++/nodes/node.cc b/libxml++/nodes/node.cc
index 7cf34d3..648424c 100644
--- a/libxml++/nodes/node.cc
+++ b/libxml++/nodes/node.cc
@@ -7,6 +7,7 @@
#include <libxml++/nodes/element.h>
#include <libxml++/nodes/node.h>
#include <libxml++/exceptions/internal_error.h>
+#include <libxml++/document.h>
#include <libxml/xpath.h>
#include <libxml/xpathInternals.h>
#include <libxml/tree.h>
@@ -32,8 +33,14 @@ const Element* Node::get_parent() const
Element* Node::get_parent()
{
- return cobj()->parent && cobj()->parent->type == XML_ELEMENT_NODE ?
- static_cast<Element*>(cobj()->parent->_private) : 0;
+ if(cobj()->parent && cobj()->parent->type == XML_ELEMENT_NODE)
+ {
+ if(cobj()->parent->_private==NULL)
+ Document::create_wrapper(cobj()->parent);
+ return static_cast<Element*>(cobj()->parent->_private);
+ }
+ else
+ return 0;
}
const Node* Node::get_next_sibling() const
@@ -43,8 +50,14 @@ const Node* Node::get_next_sibling() const
Node* Node::get_next_sibling()
{
- return cobj()->next ?
- static_cast<Node*>(cobj()->next->_private) : 0;
+ if(cobj()->next)
+ {
+ if(cobj()->next->_private==NULL)
+ Document::create_wrapper(cobj()->next);
+ return static_cast<Node*>(cobj()->next->_private);
+ }
+ else
+ return 0;
}
const Node* Node::get_previous_sibling() const
@@ -54,8 +67,14 @@ const Node* Node::get_previous_sibling() const
Node* Node::get_previous_sibling()
{
- return cobj()->prev ?
- static_cast<Node*>(cobj()->prev->_private) : 0;
+ if(cobj()->prev)
+ {
+ if(cobj()->prev->_private==NULL)
+ Document::create_wrapper(cobj()->prev);
+ return static_cast<Node*>(cobj()->prev->_private);
+ }
+ else
+ return 0;
}
Node::NodeList Node::get_children(const Glib::ustring& name)
@@ -67,20 +86,11 @@ Node::NodeList Node::get_children(const Glib::ustring& name)
NodeList children;
do
{
- if(child->_private)
+ if(name.empty() || name == (const char*)child->name)
{
- if(name.empty() || name == (const char*)child->name)
- children.push_back(reinterpret_cast<Node*>(child->_private));
- }
- else
- {
- //This should not happen:
- //This is for debugging only:
- //if(child->type == XML_ENTITY_DECL)
- //{
- // xmlEntity* centity = (xmlEntity*)child;
- // std::cerr << "Node::get_children(): unexpected unwrapped Entity Declaration node name =" << centity->name << std::endl;
- //}
+ if(child->_private==NULL)
+ Document::create_wrapper(child);
+ children.push_back(reinterpret_cast<Node*>(child->_private));
}
}
while((child = child->next));
@@ -102,7 +112,11 @@ Element* Node::add_child(const Glib::ustring& name,
_xmlNode* node = xmlAddChild(impl_, child);
if(node)
+ {
+ if(node->_private==NULL)
+ Document::create_wrapper(node);
return static_cast<Element*>(node->_private);
+ }
else
return 0;
}
@@ -120,7 +134,11 @@ Element* Node::add_child(xmlpp::Node* previous_sibling,
_xmlNode* node = xmlAddNextSibling(previous_sibling->cobj(), child);
if(node)
+ {
+ if(node->_private==NULL)
+ Document::create_wrapper(node);
return static_cast<Element*>(node->_private);
+ }
else
return 0;
}
@@ -138,7 +156,11 @@ Element* Node::add_child_before(xmlpp::Node* next_sibling,
_xmlNode* node = xmlAddPrevSibling(next_sibling->cobj(), child);
if(node)
+ {
+ if(node->_private==NULL)
+ Document::create_wrapper(node);
return static_cast<Element*>(node->_private);
+ }
else
return 0;
}
@@ -209,6 +231,8 @@ Node* Node::import_node(const Node* node, bool recursive)
#endif //LIBXMLCPP_EXCEPTIONS_ENABLED
}
+ if(imported_node->_private==NULL)
+ Document::create_wrapper(imported_node);
return static_cast<Node*>(imported_node->_private);
}
@@ -292,6 +316,8 @@ static NodeSet find_impl(xmlXPathContext* ctxt, const Glib::ustring& xpath)
//TODO: Check for other cnode->type values?
+ if(cnode->_private==NULL)
+ Document::create_wrapper(cnode);
Node* cppNode = static_cast<Node*>(cnode->_private);
nodes.push_back(cppNode);
}
diff --git a/libxml++/parsers/textreader.cc b/libxml++/parsers/textreader.cc
index a0b71c6..e5cb971 100644
--- a/libxml++/parsers/textreader.cc
+++ b/libxml++/parsers/textreader.cc
@@ -2,6 +2,7 @@
#include <libxml++/exceptions/internal_error.h>
#include <libxml++/exceptions/parse_error.h>
#include <libxml++/exceptions/validity_error.h>
+#include <libxml++/document.h>
#include <libxml/xmlreader.h>
@@ -298,7 +299,11 @@ Node* TextReader::get_current_node()
{
xmlNodePtr node = xmlTextReaderCurrentNode(impl_);
if(node)
+ {
+ if(node->_private==NULL)
+ Document::create_wrapper(node);
return static_cast<Node*>(node->_private);
+ }
check_for_exceptions();
return 0;
@@ -308,7 +313,11 @@ const Node* TextReader::get_current_node() const
{
xmlNodePtr node = xmlTextReaderCurrentNode(impl_);
if(node)
+ {
+ if(node->_private==NULL)
+ Document::create_wrapper(node);
return static_cast<Node*>(node->_private);
+ }
check_for_exceptions();
return 0;
@@ -328,7 +337,12 @@ Node* TextReader::expand()
{
xmlNodePtr node = xmlTextReaderExpand(impl_);
if(node)
+ if(node)
+ {
+ if(node->_private==NULL)
+ Document::create_wrapper(node);
return static_cast<Node*>(node->_private);
+ }
check_for_exceptions();
return 0;
diff --git a/libxml++/validators/dtdvalidator.cc b/libxml++/validators/dtdvalidator.cc
index 7d7d200..05a75db 100644
--- a/libxml++/validators/dtdvalidator.cc
+++ b/libxml++/validators/dtdvalidator.cc
@@ -13,6 +13,7 @@
#include "libxml++/keepblanks.h"
#include "libxml++/exceptions/internal_error.h"
#include "libxml++/io/istreamparserinputbuffer.h"
+#include "libxml++/document.h"
#include <libxml/parserInternals.h>//For xmlCreateFileParserCtxt().
@@ -67,6 +68,8 @@ void DtdValidator::parse_subset(const Glib::ustring& external,const Glib::ustrin
#endif //LIBXMLCPP_EXCEPTIONS_ENABLED
}
+ if(dtd->_private==NULL)
+ Document::create_wrapper(reinterpret_cast<xmlNode*>(dtd));
dtd_ = static_cast<Dtd*>(dtd->_private);
}
@@ -95,6 +98,8 @@ void DtdValidator::parse_stream(std::istream& in)
#endif //LIBXMLCPP_EXCEPTIONS_ENABLED
}
+ if(dtd->_private==NULL)
+ Document::create_wrapper(reinterpret_cast<xmlNode*>(dtd));
dtd_ = static_cast<Dtd*>(dtd->_private);
}
--
1.7.2.3
From dfaa8e46545dc3606f6f5ce558e4022c0dc3000f Mon Sep 17 00:00:00 2001
From: Alessandro Pignotti <a pignotti sssup it>
Date: Fri, 5 Nov 2010 05:11:49 +0100
Subject: [PATCH 3/4] Add free_wrappers function to Document to recursively delete libxml++ instances
---
libxml++/document.cc | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++
libxml++/document.h | 5 ++++
2 files changed, 64 insertions(+), 0 deletions(-)
diff --git a/libxml++/document.cc b/libxml++/document.cc
index 7860ba4..3ee404c 100644
--- a/libxml++/document.cc
+++ b/libxml++/document.cc
@@ -124,6 +124,65 @@ Document::~Document()
xmlFreeDoc(impl_);
}
+void Document::free_wrappers(xmlAttr* attr)
+{
+ //Delete first the local one
+ delete reinterpret_cast<Attribute*>(attr->_private);
+ attr->_private=NULL;
+ assert(attr->children->next==NULL);
+ free_wrappers(attr->children);
+}
+
+void Document::free_wrappers(xmlDoc* doc)
+{
+ //Walk the children list
+ for(xmlNode* child=doc->children; child; child=child->next)
+ free_wrappers(child);
+}
+
+void Document::free_wrappers(xmlDtd* dtd)
+{
+ //Delete the local one
+ delete reinterpret_cast<Dtd*>(dtd->_private);
+ dtd->_private=NULL;
+
+ //Walk the children list
+ for(xmlNode* child=dtd->children; child; child=child->next)
+ free_wrappers(child);
+}
+
+void Document::free_wrappers(xmlNode* node)
+{
+ //Walk the children list
+ for(xmlNode* child=node->children; child; child=child->next)
+ free_wrappers(child);
+
+ //Delete the local one
+ assert(node->type!=XML_DOCUMENT_NODE);
+ switch(node->type)
+ {
+ //Node types that have no properties
+ case XML_DTD_NODE:
+ delete reinterpret_cast<Dtd*>(node->_private);
+ node->_private=NULL;
+ return;
+ case XML_ATTRIBUTE_NODE:
+ case XML_ELEMENT_DECL:
+ case XML_ATTRIBUTE_DECL:
+ case XML_ENTITY_DECL:
+ delete reinterpret_cast<Node*>(node->_private);
+ node->_private=NULL;
+ return;
+ default:
+ delete reinterpret_cast<Node*>(node->_private);
+ node->_private=NULL;
+ }
+
+ //Walk the attributes list
+ for(xmlAttr* attr=node->properties; attr; attr=attr->next)
+ free_wrappers(attr);
+}
+
Glib::ustring Document::get_encoding() const
{
Glib::ustring encoding;
diff --git a/libxml++/document.h b/libxml++/document.h
index b0e0066..1da52eb 100644
--- a/libxml++/document.h
+++ b/libxml++/document.h
@@ -172,6 +172,11 @@ public:
///Construct the right C++ instances for a given element
static void create_wrapper(_xmlNode* node);
+ ///Recursively destroy the created C++ instances
+ static void free_wrappers(_xmlDtd* attr);
+ static void free_wrappers(_xmlDoc* attr);
+ static void free_wrappers(_xmlAttr* attr);
+ static void free_wrappers(_xmlNode* attr);
protected:
/** Retrieve an Entity.
* The entity can be from an external subset or internally declared.
--
1.7.2.3
From 72a2e85e5aa20e84129fce3d1dbbb3c1e5ae8d47 Mon Sep 17 00:00:00 2001
From: Alessandro Pignotti <a pignotti sssup it>
Date: Fri, 5 Nov 2010 05:17:25 +0100
Subject: [PATCH 4/4] Use free_wrappers to clean up the allocated instances
---
libxml++/document.cc | 1 +
libxml++/nodes/node.cc | 2 ++
libxml++/validators/dtdvalidator.cc | 5 ++++-
3 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/libxml++/document.cc b/libxml++/document.cc
index 3ee404c..7a045d6 100644
--- a/libxml++/document.cc
+++ b/libxml++/document.cc
@@ -121,6 +121,7 @@ Document::Document(xmlDoc* doc)
Document::~Document()
{
+ free_wrappers(impl_);
xmlFreeDoc(impl_);
}
diff --git a/libxml++/nodes/node.cc b/libxml++/nodes/node.cc
index 648424c..2064039 100644
--- a/libxml++/nodes/node.cc
+++ b/libxml++/nodes/node.cc
@@ -201,6 +201,7 @@ void Node::remove_child(Node* node)
{
//TODO: Allow a node to be removed without deleting it, to allow it to be moved?
//This would require a more complex memory management API.
+ Document::free_wrappers(node->cobj());
xmlUnlinkNode(node->cobj());
xmlFreeNode(node->cobj()); //The C++ instance will be deleted in a callback.
}
@@ -222,6 +223,7 @@ Node* Node::import_node(const Node* node, bool recursive)
xmlNode* added_node = xmlAddChild(this->cobj(),imported_node);
if (!added_node)
{
+ Document::free_wrappers(imported_node);
xmlFreeNode(imported_node);
#ifdef LIBXMLCPP_EXCEPTIONS_ENABLED
diff --git a/libxml++/validators/dtdvalidator.cc b/libxml++/validators/dtdvalidator.cc
index 05a75db..2c3528b 100644
--- a/libxml++/validators/dtdvalidator.cc
+++ b/libxml++/validators/dtdvalidator.cc
@@ -107,7 +107,10 @@ void DtdValidator::release_underlying()
{
if(dtd_)
{
- xmlFreeDtd(dtd_->cobj());
+ //Make a local copy as the wrapper is destroyed first
+ xmlDtd* dtd=dtd_->cobj();
+ Document::free_wrappers(dtd);
+ xmlFreeDtd(dtd);
dtd_ = 0;
}
}
--
1.7.2.3
Attachment:
signature.asc
Description: This is a digitally signed message part.