[boost] [property_tree] Review

From: Daniel Wesslén (daniel_at_[hidden])
Date: 2006-04-25 05:44:30

 >       * Are you knowledgeable about the problem domain?

Yes.

Most of my applications use either a simple XML library or a 
PersistentValue template, mapping values to entries in a ini-like 
configuration file, these values are edited both by hand and through 
user action in the application - precisely the problem this library is 
intended to solve. My XML library has an interface very much like that 
of ptree and is used in the same way.

I have been very interested in this library since Marcin first mentioned 
it in this list, and read through the documentation and part of the 
implementation back then.

 >       * What is your evaluation of the design?

The interface is "good enough" but could be better. I believe separating 
the path handling will be a big step.

Some have suggested making functions nonmembers, but I don't see that as 
an improvement. Getting and setting the contents of the tree is /the/ 
use of the tree. If we don't use member functions for that, then what do 
we use them for at all?

Regarding the parsers, the metadata for XML and Windows registry could 
be better handled. These formats both have two ways of storing data 
under a node, in property_tree represented by the <xmlattr> and \values 
subtrees. Couldn't these be handled in the same way? Both to make things 
(paths especially) more format-agnostic, and so that the next format 
with a similar facility doesn't invent a third way to handle the same thing.

 >       * What is your evaluation of the implementation?

I didn't look at it much this time.

 >       * What is your evaluation of the documentation?

Others have mentioned it needs work, especially in the rationale. Given 
the confusion at the start of the review period this is clearly the case.

 >       * What is your evaluation of the potential usefulness
 >         of the library?

Very useful for those of us who prefer to store data this way.
I use the one or both of the two previously mentioned components in 
essentially all my nontrivial applications.

 >       * Did you try to use the library? With what compiler?
 >         Did you have any problems?

I replaced a home-brewn name-value map template used for storing 
application settings. Installing the library and glancing at the 
documentation included, the whole thing took 15-20 minutes. A 
significant portion of that time was spent renaming the parameters. 
There were no problems. I used Visual C++ 7.1.

The original class was non-hierarchical and stored the values in a 
ini-like format. With some tweaks to the parameter names and a switch to 
ptree's JSON parser, the conceptual hierarchy was made explicit, making 
some (unfrequent but necessary) hand-editing very slick compared to the 
previous format where it was trivial but tedious.

 >       * How much effort did you put into your evaluation?
 >         A glance? A quick reading? In-depth study?

A glance of the documentation prior to the test mentioned above, 
followed by a quick reading after. I have read the documentation of a 
previous version in-depth, and then made a quick reading of the 
implementation (which needed changes but seems to have matured since then.)

 > - was the library suitable for your daily xml-tasks? If not, what was
 > missing?

No. I tend to use namespaces and XInclude, which the supplied XML parser 
does not support.

 > - was the library's performance good enough? If not, can you suggest
 > improvements?

I didn't use the library for anything performance-critical.

 > - was the library's design flexible enough? If not, how would you
 > suggest it should be redesigned to broaden its scope of use?

It was "flexible enough", but I would prefer if it were more flexible. 
In particular, I definitely think path handling should be separated out 
from the accessor functions, but I believe this is already in 
progress(?) That way those of us who prefer more XPath-like path 
handling could implement that as a new path-parser.
I find Ryan Gallagher's recent suggestion about separating path handling 
into its own concept very interesting but am unsure if it may be 
complicating things too much. It would certainly make accessing the tree 
very flexible, something I can only consider a good thing.

The library implements a single container type, with parsers for many 
different formats. Some of these formats have conflicting natural (or 
perhaps "traditional") ways of referring to the contained data. Allowing 
users to refer to the data in this natural way is important as I expect 
it will be far more common to use subsets of the library as "a simple 
XML parser" / "a JSON parser" / "a INI parser", etc. than to be 
format-agnositc (especially since some formats store metadata in the 
tree, which will not make sense in other formats.)

 > And finally, every review should answer this question:
 >
 >       * Do you think the library should be accepted as a Boost
 >         library?
 >         Be sure to say this explicitly so that your other comments
 >         don't obscure your overall opinion.
 >

This is a hard question. The library is good enough, and I would like to 
see it in Boost. It seems to me it can become significantly better 
though. This library has generated a very active discussion, and 
suggestions are still coming in for how to make it better.

How many / how big changes are allowed to a library once it has been 
accepted?
I wouldn't want potential improvements to be ruled out on the grounds 
that it has been accepted in its current form.

Eventually: Yes.
At the moment: I leave it to the review manager to decide if the library 
will need a second review before acceptance, but if I have to say 
something it would be a "weak no."

I will be using this library even if it gets rejected.

-- 
Daniel Wesslén