Re: [boost] [Boost Review] Property Tree Library

From: Marcin Kalicinski (kalita_at_[hidden])
Date: 2006-04-20 08:45:27

Hi Gennadiy,

Thank you for the review.

>  First and foremost I would like to remind everybody that we already have 
> one library intended to cover this problem domain (completely unacceptable 
> IMO - but that's another story).

You are talking about Program Options library. I don't quite agree it covers 
the same problem domain. I feel it is more focued on command-line than on 
reading configuration in general. The biggest difference is that 
property_tree is a DOM, not a linear structure. In program options you 
cannot easily take a only a piece of configuration and pass it to another 
component, like in the example below:

<configuration1>
    <option1>value1</option1>
    <option2>value2</option2>
</configuration1>
<configuration2>
    <option1>value1</option1>
    <option2>value2</option2>
</configuration2>

Now suppose you have a component which is only interested in option1 and 
option2:

ptree &config1 = get_child("configuration1");
ptree &config2 = get_child("configuration2");
use_my_component(config1);        // Use component with config #1
use_my_component(config2);        // Use component with config #2

With program options you would have to teach my_component extract values 
from whatever place they are in config file(s).

Other things that are not supported by program_options:

- writing configuration
- XML, JSON, INI, Windows registry parsing out of the box

Also, I think that in simple cases, syntax offered by property_tree is 
simpler and has less steep learning curve.

> 1. Name is really bad. Originally I thought this submission has something 
> to do with property_map and was surprised to see what I see.

Name comes from Java Properties class, I think problem domains of these are 
quite close, although property_tree is somewhat more sophisticated.

> 2. Why would you need fast lookup? In 99.9% of cases each variable is 
> accessed single time in a program lifetime. No need to optimize so that 
> you

I think you make a mistake of assuming that this library is only supposed to 
be used only as a reader for command-line or other startup options (truth is 
that the tutorial might suggest it). Think of it like a lightweight 
replacement for Microsoft XML parser, or JSON API, or Registry functions, or 
Windows INI file API. All of these have fast lookups, and there are good 
reasons for it. Command line parsing is there only for the sake of 
completness, it covers maybe less than 5% of the library problem domain. I 
actually added it because I thought it can make parsing _simple_ commandline 
options schemes extremely easy.

> program startup time go from 5 mls to 5 mks. You won't notice this anyway 
> (And in reality it's not even that big difference. Most of your startup 
> time you will spend initiating either network connection or some GUI 
> component or similar)

Your GUI component might be initialized by reading and looking up data in 
property tree. In one of my previous projects GUI layout was stored in 
property trees.

> 3. Even if you insist on alternative lookup. Why not just use single 
> typedef with multi_index instead of all the implementation.

Could you elaborate more on that? I considered use of multi_index to 
implement indexing for properties, but it only affected the implementation 
part of library, not interface, and because I already had a working, 
exception safe solution, I didn't see the reason to dump it and add another 
dependency on another library.

> 5. CLA parser is a joke. It's unacceptable any way you present it.

Could you please be more specific.

Anyway, bear in mind that cmdline parser is not intended to be a generic 
parser for all command-line parsing tasks, a replacement for program 
options. This is clearly stated in the docs. Its first and foremost 
advantage is that it is very simple to use, easier than program options in 
simple cases:

Can you beat that simplicity (4 lines of code)? :

boost::property_tree::ptree pt;
read_cmdline(argc - 1, argv + 1, "-", pt);

// Gets -Nxx where xx is integer
int n = pt.get<int>("N", 0);

// Tests if -fno-frills is present
bool no_frills = pt.get_optional<std::string>("f.no-frills");

> 6. I personally believe that inline implementation in this case is 
> actually is rather a disadvantage. I would be perfectly happy to stick to 
> ASCII for configuration purposes. Even if you insist on wide char support, 
> I would implement it as a thin template wrapper around offline 
> implementation that operates with void*. Plus some runtime polymorphism 
> could be used.

I don't agree. Templating on character type is very easy and dropping that 
does not buy us anything. To be flexible the library needs to support 
policy-customization anyway, so cpp implementation is not an option. Also, 
large part of the member function are templated on extracted type so they 
couldn't be moved to cpp file.

> 7.  ptree_utils reinvent the wheel. Why not use Boost string algorithms?

Agreed. That is implementation detail, could be changed later.

> 8. I keep repeating: Traits could not be template parameters. You should 
> had have three template parameter KeyType, ValueType and ComparePolicy. 
> The same way as std::map is doing

What about Extractor and Inserter? How about a proposed path splitting 
policy to allow non-string keys? I think it is better if all that is 
condensed into one class, because changing one element leads in most cases 
to changing the rest as well (if you change data type you have to provide 
different extractor and inserter, if you change key type you have to provide 
different comparison policy). std::map is a different case because there 
template parameters are much more independent.

> 9. Access interface are lacking. If I got string identify subsystem and 
> string that identify parameter separately why do I need to concatenate 
> them to get to the value

You don't have to concatenate them:

int subsystem_parameter = pt.get_child("subsystem").get<int>("parameter");

> 10. General note: the whole design/implementation is unnecessary 
> complicated. You general part is about 50k. In Boost.Test utils section I 
> have a component with very similar rationale but better design (IMO 
> obviously) implemented in 16k.

I don't see a way I could simplify it considerably. If you see, please let 
me know how.

> 11. Generating part if the design/implementation is completely 
> unnecessary. It may? be useful in 1% of the usage cases and in those cases 
> I would stick to some alternatives.

I don't understand what you mean. Can you please rephrase?

Thank you,
Marcin