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
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
