Boost logo

Boost :

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
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


> 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...

Thanks for the feedback !


Boost list run by bdawes at, gregod at, cpdaniel at, john at