From: John Maddock (john_at_[hidden])
Date: 2006-04-24 09:04:45
OK, here's my review of the property tree library, actually quite a
difficult review to write given all the hyperbole that's gone before. Still
that's better than not having anyone interested ! :-)
First of all I do not believe that the potential uses of this library are so
similar to the serialisation or program options library that it should be
excluded on those grounds.
Secondly the library looks mature and usable enough that I would consider
using it to manage program configuration.
However, I do have problems with the conceptual scope of the library as
described in the docs, more on this later but it's not the generic tree that
the docs give the impression of it being on first view.
I also have problems with the interface: that it's needlessly idiosyncratic
in places. Not so much as to render it unusable at all, but I believe better
can be done.
OK down to nut's and bolts, first off the interface:
1) As others have commented, there are too many member functions IMO. I
don't believe that changing to non-members offers any benefit in this case
(and may actually be harmful due to ADL).
Let's start with child access, I can't believe you're not using operator
here. Yes I know it's cute, but it does work well for std::map.
b[a][b][c] = 2; // assigns value 2 to node at a/b/c
The way I'm figuring this would work is:
* Accessing a child node with operator always succeeds but may point to a
"ghost" node that doesn't actually exist.
* Assignment to a node always succeeds, the node gets created if required.
* Reading from a node throws if the node doesn't exist yet, or if the
conversion can't be performed.
* If you read a Boost::optional then it shouldn't throw, and no need for a
special member function.
I'm assuming that operator  returns another "property map" by value BTW.
You could have a single function that returns the node from a path of some
kind as well, but *please* make the separator parameter the second argument
with a default parameter.
BTW, you can do much the same thing with the / operator (as has already been
suggested), boost::filesystem makes this work rather well, and I guess I'd
be just as happy with:
property-map m = mymap / "he" / "she" / "it";
int val = (mymap / "he" / "she" / "it").get<int>();
And finally if a property-tree is usable in boolean contexts you can detect
whether a node actually exists already, or is a "ghost" node with nothing in
// read in some data....
tree-type b = a["ha"]["ha"];
// do something...
I'm inclined to suggest an unreasonably "cute" interface here: just provide
a template conversion operator so that:
int i = mymap["fee"]["far"]["thumb"];
does what you would expect and throws if the node/value doesn't exist or the
conversion to int can't be done.
boost::optional opt = mymap["fee"]["far"]["thumb"];
would never throw, and
int i = mymap["fee"]["far"]["thumb"].get(3);
returns 3 if there is no data.
A step to far? Maybe, and I would be perfectly happy with a single get()
function that did all the getting, again not throwing if the type being
fetched is a boost::optional, and with a single argument overload version
for data with default values.
You must know what's coming... what's wrong with operator = ?
OK you may need a member function to deal with std::locale, but you know
what, I bet 99.999% of programmers would be completely happy with just the
default (or the locale could be a property of the property-tree as a whole -
just add one imbue() member function and use it's locale everywhere). Of
course if the argument to operator= is another property tree, then it copies
the tree to that node rather than setting the data.
The comments so far relate to any kind of hierarchical "tree like"
associative storage, but now for the real guts of the issue for me (and I
think why this library has attracted so much comment).
What is this library trying to be?
Yes, I've seen you're posts, and I believe I do understand what it's for :-)
But that's not the point, the docs and actually the interface are at odds
with what it's for. There is definitely a place in boost for a generic tree
container. But this isn't one as you have said many times I believe, but to
look at the interface it's kind of pretending to be one :-)
So what is this library? I believe it's an interface to a hierarchical
database, in fact I believe it need not be an in memory container at all. I
certainly wouldn't try and read (and then write back!!!) the entire windows
registry, or any large part of it, with this library. And yet, why
shouldn't it be able to provide a view onto large data sets that form some
kind of hierarchy? I'm thinking out loud here, but what would happen if
your read/write functions were replaced by some kind of opaque data access
API, that the tree used internally to access data from it's actual source.
Possibly this is a step too far, but it may be food for thought anyway.
I guess what I'm saying is either lets have:
* A generic tree data structure with serialisation to various formats (not
necessarily using boost.serialisation depending on practicalities).
* A wrapper around external hierarchical databases, that doesn't necessarily
load data into memory.
I believe we need a library like this in Boost. And I believe this one is -
in spite of what I've said above - perfectly usable. However, I believe a
better interface is possible. So, I'm going to vote weakly "no": for the
review manager that means this is not an "over-my-dead-body" vote, it's a
"can-do-better on the interface, but let it through if there's a consensus
Regards, John Maddock.
Boost list run by bdawes at acm.org, david.abrahams at rcn.com, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk