[boost] [property tree][review]

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.

How about:

mymap b;
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";

or

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

tree-type a;
// read in some data....
tree-type b = a["ha"]["ha"];
if(b)
{
  // do something...
}

Reading data

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.

Setting data:

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.

Conclusions:

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 
for it".

Whew.

Regards, John Maddock.