Re: [boost] API Review request: XML APIs for C++

From: Stefan Seefeld (seefeld_at_[hidden])
Date: 2003-06-12 16:12:01

Hamish Mackenzie wrote:
> Looks good.  I have only looked at the dom stuff so far.  Why are you
> storing information in _private?  What goes in there that could not be
> extracted when the nodes are accessed?  It seems like a lot of
> unnecessary overhead.

I'm wrapping libxml2. The structs provided by libxml2 all carry a
'_private' member, precisely because it's a good way for extensions
such as language wrapping.
libxml2 itself calls callbacks whenever it allocates instances of these
structs, and I allocate my C++ wrappers in these callbacks, and let the
_private member point to it.
That way I'v got a pointer from the C struct to the C++ wrapper
(_private), as well as a pointer from the C++ wrapper to the C struct
(my_impl).
Looking up this  node's parent node is thus simply

static_cast<Node *>(this->my_impl->parent->_private);

> Here are some suggestions...
> 
> 1) parse_file could be
> 
> class parse_file
> {
> public:
>   parse_file( const std::string & ) : {}
>   ...
> };

making 'parse_file' a class suggests it is carrying some data/state.
What would that be ? I'm thinking of 'parse_file' as a stateless
factory, i.e. a function returning a newly created document.

> 2) How about parse_string (based on xmlParseMemory)?

hmm, while that would be possible, I think it's more C++'ish to
provide document extraction from a std::streambuf, which could be
a string_buf or any other buffer implementation. (Note that I wouldn't
use std::iostreams as that would suggest that formatted extraction is
possible, which would only work on ascii, not on unicode content.

> 3) Why dom::basic_document::clone?  Why not have the copy constructor
> and assignment operator should do a deep copy of the document?  This is
> consistent with other containers. If you want to stick with clone return
> an auto_ptr and and derive basic_document from boost::noncopyable.

fair enough.

> 4) How about something like
> 
> class basic_document
> {
> public:
>   ...
> 
>   typedef node_iterator iterator;
>   typedef const_node_iterator const_iterator;
> 
>   iterator begin()
>     { return iterator( ptr_->children ); }
>   const_iterator begin() const
>     { return const_iterator( ptr_->children ); }
>   iterator end() { return iterator(); }
>   const_iterator end() const { return const_iterator(); }
> 
>   iterator root()
>     { return iterator( xmlDocGetRootElement( ptr_ ) ); }
>   const_iterator root() const
>     { return const_iterator( xmlDocGetRootElement( ptr_ ) ); }
> };

I don't understand: are you suggesting an iterator that traverses
the whole tree (as opposed the children of a single node) ?

While that would be possible, I don't think it would actually
be useful. I'v written a Visitor which I use to look up specific
nodes. As the node 'type system' is known without C++ RTTI that
doesn't require double dispatching though...

> 5) basic_xpath and basic_xpath_result should be derived from
> boost::noncopyable

ok

> 6) Leading _ chars are a no no (reserved for compiler implementors).  I
> know _private is defined in libxml2, but what you see below is not...

ok
Thanks for the feedback !

Regards,
                Stefan