Re: [boost] API Review request: XML API for C++, second round

From: Hamish Mackenzie (hamish_at_[hidden])
Date: 2003-06-26 03:19:15

On Wed, 2003-06-25 at 20:45, Stefan Seefeld wrote: 
> > Why should the node-wrappers keep the document alive?
> 
> for consistency, and convenience. In the same way you can get down from
> the document to the individual nodes you can get up: node.parent() and
> node.document() provide the means to walk up towards the document root.

And yet it does not keep it's parent() alive.  How is that consistent?  

In what way would you be unable to walk toward the document if it was
not add reffed?

> Imagine a factory object that is initialized with a dom::node_ptr
> (holding configurational data, say). Whenever you access its method,
> the object looks up data in the node...
> 
> 
> class factory
> {
> public:
>    factory(dom::node_ptr n) : my_node(n) {}
>    foo create_foo(); /* access my_node */
>    bar create_bar(); /* access my_node */
> private:
>    dom::node_ptr my_node;
> };
> 
> factory 'owns' the node, so whereever you instantiate the factory,
> you'd read in a dom::document, look up the relevant node, and pass
> that to the constructor:
> 
> factory *f;
> {
>    dom::document_ptr document("config.xml");
>    dom::node_set set = document.root_node().find("//factory.info");
>    f = new factory(set[0]);
> } // document and set are now deleted, but factory still references
>    // the document through its 'my_node' member
> 
> 
> If the document wasn't ref-counted, you'd need to pass it along with
> the node to the factory, as only the factory would know when to drop
> it (in its destructor, presumably).

One of the following should be ok...

// Each factory has its own document
class factory : boost::noncopyable
{
public:
  factory( ... );
private:
  dom::node_ptr node_;
  std::auto_ptr< dom::document > doc_;
};

// They share documents
class factory
{
public:
  factory( ... );
private:
  dom::node_ptr node_;
  std::shared_ptr< dom::document > doc_;
};

// All factories use the same document
class factory_ref
{
private:
  factory_ref( dom::node_ptr node ) : node_( node )
  {
  }
  dom::node_ptr node_;
  friend class factories;
};

class factories : boost::noncopyable
{
public:
  factory_ref get_factory( ... );

private:
  std::auto_ptr< dom::document > doc_;
};

By making the user manage the document object it forces them to consider
which of these models they want.

With the implicit add ref it is hard to tell what the intent of the code
is.  Your example illustrates this problem well, but consider this
simpler version....

{
xml::document_ptr doc( "config.xml" );
some_function( doc->root_node() );
}

I cannot tell what the writer intended the scope of doc to be without
examining some_function and understanding what it does.  It might add
ref the document it might not.

Now without the add ref stuff we could have 

{
xml::document doc( "config.xml" );
some_function( doc->root_node() );
}

and

{
std::auto_ptr< xml::document > doc( new xml::document( "config.xml" ) );
some_function( doc, doc->root_node() );
}

Now we know if some_function is allowed to extend the life of the
document or not.

> > Here is the analogy I think works best...
> > 
> > container --> document
> > container::value_type --> node
> > container::iterator --> node_iterator
> > container::pointer_type --> node_pointer
> > container::reference_type --> node_reference
> 
> hmm, that makes it look simpler than it actually is: is there really
> a single 'value_type' ?

True you need attribute_, element_ etc. variations which you already
have.

value_types could exist but it would require a deep copy to be
consistent.  If you do want to define it then I suggest

typedef void value_type;

Along with an explanation.  That way if I write

node_iterator::value_type temp( *i ); // Make a copy for later
(*i).some_non_const_function();

I would get a compiler error rather than a nasty surprise when I use
temp.

> Is there really a single iterator ? (iterating 
> over all child nodes of a given parent and iterating over all attributes
> is not the same)

Your iterator types look good.  Why is there an extra level of
indirection in basic_element_const_iterator?

> Also, what is a node_reference (as opposed to a pointer) ?
If you renamed your _ptr classes as _ref or _reference and replace _ptr
with instances of basic_node_pointer.  (Make basic_node_pointer by
taking basic_element_iterator and stripping the ++ and -- operators).

Then some_pointer->x() and some_iterator->x() would call the same x()
member of the _reference class.

-- 
Hamish Mackenzie <hamish_at_[hidden]>