[libxml++] Preservation of deep const-ness

[Knut Aksel Røysland <knutroy ifi uio no>]

Hi.

[This is more or less a copy of an email I tried to send to this list 
about a month and a half ago. I am now a member of this list, and hope 
this one gets through.]

I have been using libxml++ for a while and I find its API pleasant
to work with.

When it comes to preservation of const-ness, there appears to be some 
inconsistencies between querying for certain nodes, like pointers to 
parents or siblings, and querying for node collections containing 
pointers to child or attribute nodes.

Currently the const-qualified member functions used for querying for 
children and attributes,

   const NodeList Node::get_children(const Glib::ustring &) const
   const AttributeList Element::get_attributes() const

return STL containers that are const-qualified, while containing 
non-const pointers to the objects (children or attributes). As a 
consequence, client code may modify those objects, undermining the 
const-qualification of the parent node. On the other hand, client code 
is prohibited from modifying siblings or parents, since these pointers 
are const-qualified when returned from the respective const member 
functions.

The enclosed patch contains a proposed fix for this situation.

In addition, the two "find" member functions of Node have been 
overloaded to work with both non-const and const Node objects, returning 
std::vector<Node*> (NodeSet) and std::vector<const Node*> 
(ConstNodeSet), respectively.

The patch also contains an overloaded definition of 
Document::get_root_node(), preserving const from Document to root Element.

I also reorganized the code a bit to get rid of a few const_casts. 
Shared code has been extracted into static *_impl functions.

I assume that some of these changes, if approved, would have to go into 
a new major release, since these API changes are not backward compatible.

Keep up the good work with libxml++! :-)

--
Sincerely,
Knut Aksel Røysland

commit 6b2940faf811a941b2fd375901f3ba3df206e37f
Author: Knut Aksel Røysland <knutroy ifi uio no>
Date:   Sat Jan 23 14:07:09 2010 +0100

    Ensure preservation of deep const-ness for various member functions of classes "Node" and "Element".
    
    * Returned STL containers carry const pointers when returned from const objects of "Node" or "Element".
    * A bit of refactoring to get rid of some "const_cast"s.
    * Preserve const from "Document" to root "Element" when calling get_root_node() on a const document object.

diff --git a/examples/dom_parse_entities/main.cc b/examples/dom_parse_entities/main.cc
index a64b6b0..c47b2f7 100644
--- a/examples/dom_parse_entities/main.cc
+++ b/examples/dom_parse_entities/main.cc
@@ -51,8 +51,8 @@ void print_node(const xmlpp::Node* node, unsigned int indentation = 0)
   if(!nodeContent)
   {
     //Recurse through child nodes:
-    xmlpp::Node::NodeList list = node->get_children();
-    for(xmlpp::Node::NodeList::iterator iter = list.begin(); iter != list.end(); ++iter)
+    xmlpp::Node::ConstNodeList list = node->get_children();
+    for(xmlpp::Node::ConstNodeList::iterator iter = list.begin(); iter != list.end(); ++iter)
     {   
       print_node(*iter, indentation + 2); //recursive
     }
diff --git a/examples/dom_parser/main.cc b/examples/dom_parser/main.cc
index 5eacbf2..d620091 100644
--- a/examples/dom_parser/main.cc
+++ b/examples/dom_parser/main.cc
@@ -87,8 +87,8 @@ void print_node(const xmlpp::Node* node, unsigned int indentation = 0)
     std::cout << "     line = " << node->get_line() << std::endl;
 
     //Print attributes:
-    const xmlpp::Element::AttributeList& attributes = nodeElement->get_attributes();
-    for(xmlpp::Element::AttributeList::const_iterator iter = attributes.begin(); iter != attributes.end(); ++iter)
+    const xmlpp::Element::ConstAttributeList& attributes = nodeElement->get_attributes();
+    for(xmlpp::Element::ConstAttributeList::const_iterator iter = attributes.begin(); iter != attributes.end(); ++iter)
     {
       const xmlpp::Attribute* attribute = *iter;
       print_indentation(indentation);
@@ -110,8 +110,8 @@ void print_node(const xmlpp::Node* node, unsigned int indentation = 0)
   if(!nodeContent)
   {
     //Recurse through child nodes:
-    xmlpp::Node::NodeList list = node->get_children();
-    for(xmlpp::Node::NodeList::iterator iter = list.begin(); iter != list.end(); ++iter)
+    xmlpp::Node::ConstNodeList list = node->get_children();
+    for(xmlpp::Node::ConstNodeList::iterator iter = list.begin(); iter != list.end(); ++iter)
     {
       print_node(*iter, indentation + 2); //recursive
     }
diff --git a/examples/dom_parser_raw/main.cc b/examples/dom_parser_raw/main.cc
index 14e31ee..3c3f99f 100644
--- a/examples/dom_parser_raw/main.cc
+++ b/examples/dom_parser_raw/main.cc
@@ -33,8 +33,8 @@ void print_node(const xmlpp::Node* node, unsigned int indentation = 0)
   std::cout << "Node name = " << node->get_name() << std::endl;
 
   //Recurse through child nodes:
-  xmlpp::Node::NodeList list = node->get_children();
-  for(xmlpp::Node::NodeList::iterator iter = list.begin(); iter != list.end(); ++iter)
+  xmlpp::Node::ConstNodeList list = node->get_children();
+  for(xmlpp::Node::ConstNodeList::iterator iter = list.begin(); iter != list.end(); ++iter)
   {
     print_node(*iter, indentation + 2); //recursive
   }
diff --git a/examples/dom_xpath/main.cc b/examples/dom_xpath/main.cc
index d6fae13..20428ec 100644
--- a/examples/dom_xpath/main.cc
+++ b/examples/dom_xpath/main.cc
@@ -33,12 +33,12 @@ void xpath_test(const xmlpp::Node* node, const Glib::ustring& xpath)
   std::cout << std::endl; //Separate tests by an empty line.
   std::cout << "searching with xpath '" << xpath << "' in root node: " << std::endl;
 
-  xmlpp::NodeSet set = node->find(xpath);
-  
+  xmlpp::ConstNodeSet set = node->find(xpath);
+
   std::cout << set.size() << " nodes have been found:" << std::endl;
 
   //Print the structural paths:
-  for(xmlpp::NodeSet::iterator i = set.begin(); i != set.end(); ++i)
+  for(xmlpp::ConstNodeSet::iterator i = set.begin(); i != set.end(); ++i)
   {
     std::cout << " " << (*i)->get_path() << std::endl;
   }
diff --git a/examples/sax_parser_build_dom/main.cc b/examples/sax_parser_build_dom/main.cc
index f453966..7e1471d 100644
--- a/examples/sax_parser_build_dom/main.cc
+++ b/examples/sax_parser_build_dom/main.cc
@@ -57,12 +57,12 @@ main(int argc, char* argv[])
     std::cout << doc.write_to_string_formatted() << std::endl;
 
     // Use the custom DOM
-    SVG::Element* element = doc.get_root();
+    const SVG::Element* element = doc.get_root();
     std::cout << "root's name is \"" << element->get_name() << "\"" << std::endl;
-    xmlpp::NodeSet nl = element->find("//path[ style != '']");
+    xmlpp::ConstNodeSet nl = element->find("//path[ style != '']");
     if(!nl.empty())
     {
-      SVG::Path* path = dynamic_cast<SVG::Path*>(nl[0]);
+      const SVG::Path* path = dynamic_cast<const SVG::Path*>(nl[0]);
       std::cout << "style of first path node with a style = \"" << path->get_style() << "\"" << std::endl;
     }
   #ifdef LIBXMLCPP_EXCEPTIONS_ENABLED
diff --git a/examples/sax_parser_build_dom/svgdocument.cc b/examples/sax_parser_build_dom/svgdocument.cc
index db4aceb..5f96f35 100644
--- a/examples/sax_parser_build_dom/svgdocument.cc
+++ b/examples/sax_parser_build_dom/svgdocument.cc
@@ -25,9 +25,9 @@
 
 namespace SVG {
 
-SVG::Element* Document::get_root() const
+const SVG::Element* Document::get_root() const
 {
-   return dynamic_cast<SVG::Element*>(get_root_node()); // RTTI
+   return dynamic_cast<const SVG::Element*>(get_root_node()); // RTTI
 }
 
 } //namespace SVG
diff --git a/examples/sax_parser_build_dom/svgdocument.h b/examples/sax_parser_build_dom/svgdocument.h
index 324d899..37e25f0 100644
--- a/examples/sax_parser_build_dom/svgdocument.h
+++ b/examples/sax_parser_build_dom/svgdocument.h
@@ -32,7 +32,7 @@ namespace SVG {
 class Document : public xmlpp::Document
 {
 public:
-  SVG::Element* get_root() const;
+  const SVG::Element* get_root() const;
   // TODO: add custom document methods
 };
 
diff --git a/libxml++/document.cc b/libxml++/document.cc
index 1ad9f50..db94903 100644
--- a/libxml++/document.cc
+++ b/libxml++/document.cc
@@ -202,15 +202,25 @@ void Document::set_internal_subset(const Glib::ustring& name,
     dtd->_private = new Dtd(dtd);
 }
 
-Element* Document::get_root_node() const
+static Element * get_root_node_impl(_xmlDoc* doc)
 {
-  xmlNode* root = xmlDocGetRootElement(impl_);
+  xmlNode* root = xmlDocGetRootElement(doc);
   if(root == 0)
     return 0;
   else
     return reinterpret_cast<Element*>(root->_private);
 }
 
+Element* Document::get_root_node()
+{
+  return get_root_node_impl(impl_);
+}
+
+const Element* Document::get_root_node() const
+{
+  return get_root_node_impl(impl_);
+}
+
 Element* Document::create_root_node(const Glib::ustring& name,
                                     const Glib::ustring& ns_uri,
                                     const Glib::ustring& ns_prefix)
diff --git a/libxml++/document.h b/libxml++/document.h
index 826dc14..b03a3f3 100644
--- a/libxml++/document.h
+++ b/libxml++/document.h
@@ -77,7 +77,13 @@ public:
    * This function does _not_ create a default root node if it doesn't exist.
    * @return A pointer to the root node if it exists, 0 otherwise.
    */
-  Element* get_root_node() const;
+  Element* get_root_node();
+
+  /** Return the root node.
+   * This function does _not_ create a default root node if it doesn't exist.
+   * @return A pointer to the root node if it exists, 0 otherwise.
+   */
+  const Element* get_root_node() const;
 
   /** Creates the root node.
    * @param name The node's name.
diff --git a/libxml++/nodes/element.cc b/libxml++/nodes/element.cc
index 69604f5..d3165b4 100644
--- a/libxml++/nodes/element.cc
+++ b/libxml++/nodes/element.cc
@@ -20,24 +20,56 @@ Element::Element(xmlNode* node)
 Element::~Element()
 {}
 
-Element::AttributeList Element::get_attributes()
+template <typename T>
+void get_attributes_impl(const xmlNode* node, T & attributes)
 {
-  AttributeList attributes;
-  for(xmlAttr* attr = cobj()->properties; attr; attr = attr->next)
+  for(xmlAttr* attr = node->properties; attr; attr = attr->next)
   {
     attributes.push_back(reinterpret_cast<Attribute*>(attr->_private));
   }
+}
 
+Element::AttributeList Element::get_attributes()
+{
+  AttributeList attributes;
+  get_attributes_impl<AttributeList>(cobj(), attributes);
   return attributes;
 }
 
-const Element::AttributeList Element::get_attributes() const
+Element::ConstAttributeList Element::get_attributes() const
 {
-  return const_cast<Element*>(this)->get_attributes();
+  ConstAttributeList attributes;
+  get_attributes_impl<ConstAttributeList>(cobj(), attributes);
+  return attributes;
 }
 
 Attribute* Element::get_attribute(const Glib::ustring& name,
-                                  const Glib::ustring& ns_prefix) const
+                                  const Glib::ustring& ns_prefix)
+{
+  if(ns_prefix.empty())
+  {
+    xmlAttr* attr = xmlHasProp(const_cast<xmlNode*>(cobj()), (const xmlChar*)name.c_str());
+    if( attr )
+    {
+      return reinterpret_cast<Attribute*>(attr->_private);
+    }
+  }
+  else
+  {
+    Glib::ustring ns_uri = get_namespace_uri_for_prefix(ns_prefix);
+    xmlAttr* attr = xmlHasNsProp(const_cast<xmlNode*>(cobj()), (const xmlChar*)name.c_str(),
+                                 (const xmlChar*)ns_uri.c_str());
+    if( attr )
+    {
+      return reinterpret_cast<Attribute*>(attr->_private);
+    }
+  }
+
+  return 0;
+}
+
+const Attribute* Element::get_attribute(const Glib::ustring& name,
+                                        const Glib::ustring& ns_prefix) const
 {
   if(ns_prefix.empty())
   {
@@ -49,7 +81,7 @@ Attribute* Element::get_attribute(const Glib::ustring& name,
   }
   else
   {
-    Glib::ustring ns_uri = get_namespace_uri_for_prefix(ns_prefix);  
+    Glib::ustring ns_uri = get_namespace_uri_for_prefix(ns_prefix);
     xmlAttr* attr = xmlHasNsProp(const_cast<xmlNode*>(cobj()), (const xmlChar*)name.c_str(),
                                  (const xmlChar*)ns_uri.c_str());
     if( attr )
diff --git a/libxml++/nodes/element.h b/libxml++/nodes/element.h
index a64efd7..83b1976 100644
--- a/libxml++/nodes/element.h
+++ b/libxml++/nodes/element.h
@@ -23,6 +23,7 @@ public:
   virtual ~Element();
 
   typedef std::list<Attribute*> AttributeList;
+  typedef std::list<const Attribute*> ConstAttributeList;
 
   /** This adds a namespace declaration to this node which will apply to this node and all children.
    * @param ns_uri The namespace to associate with the prefix, or to use as the default namespace if no prefix is specified.
@@ -38,13 +39,19 @@ public:
   /** Obtain the list of attributes for this element.
    * @returns The list of attributes.
    */
-  const AttributeList get_attributes() const;
-    
+  ConstAttributeList get_attributes() const;
+
   // FIXME: the following only returns explicitely provided
   // attributes, not default ones declared in the dtd.
   // TOOD: Is this still true? murrayc
   Attribute* get_attribute(const Glib::ustring& name,
-                           const Glib::ustring& ns_prefix = Glib::ustring()) const;
+                           const Glib::ustring& ns_prefix = Glib::ustring());
+
+  // FIXME: the following only returns explicitely provided
+  // attributes, not default ones declared in the dtd.
+  // TOOD: Is this still true? murrayc
+  const Attribute* get_attribute(const Glib::ustring& name,
+				 const Glib::ustring& ns_prefix = Glib::ustring()) const;
 
   /** Get the value of the attribute with this name, and optionally with this namespace.
    * For finer control, you might use get_attribute() and use the methods of the Attribute class.
diff --git a/libxml++/nodes/node.cc b/libxml++/nodes/node.cc
index c3c4570..4c5360e 100644
--- a/libxml++/nodes/node.cc
+++ b/libxml++/nodes/node.cc
@@ -25,47 +25,61 @@ Node::Node(xmlNode* node)
 Node::~Node()
 {}
 
+static Element* get_parent_impl(const xmlNode* node)
+{
+  return node->parent && node->parent->type == XML_ELEMENT_NODE ?
+             static_cast<Element *>(node->parent->_private) : NULL;
+}
+
 const Element* Node::get_parent() const
 {
-  return cobj()->parent && cobj()->parent->type == XML_ELEMENT_NODE ? 
-             static_cast<const Element*>(cobj()->parent->_private) : NULL;
+  return get_parent_impl(impl_);
 }
 
 Element* Node::get_parent()
 {
-  return cobj()->parent && cobj()->parent->type == XML_ELEMENT_NODE ? 
-            static_cast<Element*>(cobj()->parent->_private) : NULL;
+  return get_parent_impl(impl_);
+}
+
+Node* get_next_sibling_impl(const xmlNode* node)
+{
+  return node->next ?
+              static_cast<Node*>(node->next->_private) : NULL;
 }
 
 const Node* Node::get_next_sibling() const
 {
-  return const_cast<Node*>(this)->get_next_sibling();
+  return get_next_sibling_impl(impl_);
 }
 
 Node* Node::get_next_sibling()
 {
-  return cobj()->next ? 
-	        static_cast<Node*>(cobj()->next->_private) : NULL;
+  return get_next_sibling_impl(impl_);
+}
+
+Node* get_previous_sibling_impl(const xmlNode* node)
+{
+  return node->prev ?
+          static_cast<Node*>(node->prev->_private) : NULL;
 }
 
 const Node* Node::get_previous_sibling() const
 {
-  return const_cast<Node*>(this)->get_previous_sibling();
+  return get_previous_sibling_impl(impl_);
 }
 
 Node* Node::get_previous_sibling()
 {
-  return cobj()->prev ? 
-            static_cast<Node*>(cobj()->prev->_private) : NULL;
+  return get_previous_sibling_impl(impl_);
 }
 
-Node::NodeList Node::get_children(const Glib::ustring& name)
+template <typename T>
+static void get_children_impl(const xmlNode * child,
+                              const Glib::ustring & name,
+                              T & children)
 {
-   xmlNode* child = impl_->children;
    if(!child)
-     return NodeList();
-
-   NodeList children;
+      return;
    do
    {
       if(child->_private)
@@ -85,13 +99,20 @@ Node::NodeList Node::get_children(const Glib::ustring& name)
       }
    }
    while((child = child->next));
-   
+}
+
+Node::NodeList Node::get_children(const Glib::ustring& name)
+{
+   NodeList children;
+   get_children_impl<NodeList>(impl_->children, name, children);
    return children;
 }
 
-const Node::NodeList Node::get_children(const Glib::ustring& name) const
+Node::ConstNodeList Node::get_children(const Glib::ustring& name) const
 {
-  return const_cast<Node*>(this)->get_children(name);
+   ConstNodeList children;
+   get_children_impl<ConstNodeList>(impl_->children, name, children);
+   return children;
 }
 
 Element* Node::add_child(const Glib::ustring& name,
@@ -247,7 +268,8 @@ Glib::ustring Node::get_path() const
   return retn;
 }
 
-static NodeSet find_impl(xmlXPathContext* ctxt, const Glib::ustring& xpath)
+template <typename T>
+static void find_impl(xmlXPathContext* ctxt, const Glib::ustring& xpath, T & nodes)
 {
   xmlXPathObject* result = xmlXPathEval((const xmlChar*)xpath.c_str(), ctxt);
 
@@ -258,7 +280,7 @@ static NodeSet find_impl(xmlXPathContext* ctxt, const Glib::ustring& xpath)
     #ifdef LIBXMLCPP_EXCEPTIONS_ENABLED
     throw exception("Invalid XPath: " + xpath);
     #else
-    return NodeSet();
+    return;
     #endif //LIBXMLCPP_EXCEPTIONS_ENABLED
   }
 
@@ -270,12 +292,11 @@ static NodeSet find_impl(xmlXPathContext* ctxt, const Glib::ustring& xpath)
     #ifdef LIBXMLCPP_EXCEPTIONS_ENABLE
     throw internal_error("Only nodeset result types are supported.");
     #else
-    return NodeSet();
+    return;
     #endif //LIBXMLCPP_EXCEPTIONS_ENABLED
   }
 
   xmlNodeSet* nodeset = result->nodesetval;
-  NodeSet nodes;
   if( nodeset )
   {
     nodes.reserve( nodeset->nodeNr );
@@ -289,31 +310,63 @@ static NodeSet find_impl(xmlXPathContext* ctxt, const Glib::ustring& xpath)
 
   xmlXPathFreeObject(result);
   xmlXPathFreeContext(ctxt);
+}
 
+template <typename T>
+static void find_impl(xmlXPathContext* ctxt,
+                   const Glib::ustring& xpath,
+                   const Node::PrefixNsMap& namespaces,
+                   T & nodes)
+{
+  for (Node::PrefixNsMap::const_iterator it=namespaces.begin();
+       it != namespaces.end(); it++)
+    xmlXPathRegisterNs(ctxt,
+                       reinterpret_cast<const xmlChar*>(it->first.c_str()),
+                       reinterpret_cast<const xmlChar*>(it->second.c_str()));
+
+  find_impl<T>(ctxt, xpath, nodes);
+}
+
+NodeSet Node::find(const Glib::ustring& xpath)
+{
+  NodeSet nodes;
+  xmlXPathContext* ctxt = xmlXPathNewContext(impl_->doc);
+  ctxt->node = impl_;
+
+  find_impl<NodeSet>(ctxt, xpath, nodes);
   return nodes;
 }
 
-NodeSet Node::find(const Glib::ustring& xpath) const
+ConstNodeSet Node::find(const Glib::ustring& xpath) const
 {
+  ConstNodeSet nodes;
   xmlXPathContext* ctxt = xmlXPathNewContext(impl_->doc);
   ctxt->node = impl_;
-  
-  return find_impl(ctxt, xpath);
+
+  find_impl<ConstNodeSet>(ctxt, xpath, nodes);
+  return nodes;
 }
 
 NodeSet Node::find(const Glib::ustring& xpath,
-		   const PrefixNsMap& namespaces) const
+                   const PrefixNsMap& namespaces)
 {
+  NodeSet nodes;
   xmlXPathContext* ctxt = xmlXPathNewContext(impl_->doc);
   ctxt->node = impl_;
 
-  for (PrefixNsMap::const_iterator it=namespaces.begin();
-       it != namespaces.end(); it++)
-    xmlXPathRegisterNs(ctxt,
-		       reinterpret_cast<const xmlChar*>(it->first.c_str()),
-		       reinterpret_cast<const xmlChar*>(it->second.c_str()));
+  find_impl<NodeSet>(ctxt, xpath, namespaces, nodes);
+  return nodes;
+}
 
-  return find_impl(ctxt, xpath);
+ConstNodeSet Node::find(const Glib::ustring& xpath,
+                   const PrefixNsMap& namespaces) const
+{
+  ConstNodeSet nodes;
+  xmlXPathContext* ctxt = xmlXPathNewContext(impl_->doc);
+  ctxt->node = impl_;
+
+  find_impl<ConstNodeSet>(ctxt, xpath, namespaces, nodes);
+  return nodes;
 }
 
 Glib::ustring Node::get_namespace_prefix() const
diff --git a/libxml++/nodes/node.h b/libxml++/nodes/node.h
index f3f4ac2..55f1fea 100644
--- a/libxml++/nodes/node.h
+++ b/libxml++/nodes/node.h
@@ -29,6 +29,7 @@ class Attribute;
 
 class Node;
 typedef std::vector<Node*> NodeSet;
+typedef std::vector<const Node*> ConstNodeSet;
 
 /** Represents XML Nodes.
  * You should never new or delete Nodes. The Parser will create and manage them for you.
@@ -37,6 +38,7 @@ class Node : public NonCopyable
 {
 public:
   typedef std::list<Node*> NodeList;
+  typedef std::list<const Node*> ConstNodeList;
 
   explicit Node(_xmlNode* node);
   virtual ~Node();
@@ -105,7 +107,7 @@ public:
    * @param name The names of the child nodes to get. If you do not specigy a name, then the list will contain all nodes, regardless of their names.
    * @returns The list of child nodes.
    */
-  const NodeList get_children(const Glib::ustring& name = Glib::ustring()) const;
+  ConstNodeList get_children(const Glib::ustring& name = Glib::ustring()) const;
 
   /** Add a child element to this node.
    * @param name The new node name
@@ -160,7 +162,12 @@ public:
   /** Find nodes from a XPath expression.
    * @param xpath The XPath of the nodes.
    */
-  NodeSet find(const Glib::ustring& xpath) const;
+  NodeSet find(const Glib::ustring& xpath);
+
+  /** Find nodes from a XPath expression.
+   * @param xpath The XPath of the nodes.
+   */
+  ConstNodeSet find(const Glib::ustring& xpath) const;
 
   /** A map of namespace prefixes to namespace URIs.
    */
@@ -170,7 +177,13 @@ public:
    * @param xpath The XPath of the nodes.
    * @param namespaces A map of namespace prefixes to namespace URIs to be used while finding.
    */
-  NodeSet find(const Glib::ustring& xpath, const PrefixNsMap& namespaces) const;
+  NodeSet find(const Glib::ustring& xpath, const PrefixNsMap& namespaces);
+
+  /** Find nodes from a XPath expression.
+   * @param xpath The XPath of the nodes.
+   * @param namespaces A map of namespace prefixes to namespace URIs to be used while finding.
+   */
+  ConstNodeSet find(const Glib::ustring& xpath, const PrefixNsMap& namespaces) const;
 
 
   ///Access the underlying libxml implementation.