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

From: Gennadiy Rozental (gennadiy.rozental_at_[hidden])
Date: 2006-04-20 11:52:56

"Marcin Kalicinski" <kalita_at_[hidden]> wrote in message 
news:e27vp4$o1p$1_at_sea.gmane.org...
> 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 focused on command-line 
> than on reading configuration in general.

Is there any facts supporting this feeling?

> The biggest difference is that property_tree is a DOM, not a linear 
> structure.

In a big part it's implementation detail. I might as well keep all the 
parameter in single list with names like "a.b.c.d". Though I agree that 
variable_map could be done better.

> 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

I am not saying PO library designed good. It should have supported 
parameters hierarchy in it's variable_map.

> 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

Why would I need that? Or rather in how many instances I would need that?

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

You could implement them as an add-ons.

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

It's matter of opinion and again it's just  PO issue.

>> 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.

Word property seems misplaced here. Why do you name this entities 
properties? It's way too generic name.
Word tree is misleading here. Tree is just an implementation detail. You may 
as well implement this using different data structure.

>> 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).

Ok. So state your problem domain.

>  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.

My understanding of problem domain is: program runtime parameters support. 
Whatever media is used doesn't matter. And both yours and PO library intends 
to cover it.

>> 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.

Majority of your startup time you will spend in a system calls anyway.

>> 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.

I believe typedef  multiindex< ... > property_tree;

and several free functions like

get( .... )

should constitute your whole interface and implementation.

>> 5. CLA parser is a joke. It's unacceptable any way you present it.
>
> Could you please be more specific.

Essentially it's unusable for anything any different for one rigid format 
you chose.

> 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.

Why do you include it then? What it is intended to be?

> 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");

Given that you did not have any way to specify CLA options I don't see it 
that simpler than PO interface.

>> 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.

Most of the things that  are implemented using compile-time polymorphism 
could be done using runtime one. You just need to tweak you implementation a 
bit.

>> 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?

Personally I don't see why do you need that at all. I would stick with 
lexical_cast. Simplicity is a virtue. But! If you do want to support an 
ability to extract/save values differently for different types, you need to 
implement this as trait not as a policy.

> 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.

How are they more independent? Are you saying that you compare function is 
somehow different then std::map one?

>> 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");

And if I have more levels?

string trace_config = 
pt.get_child("MyLib").get("debug").get("trace").get("config");

I would rather prefer

string trace_config = pt.get("MyLib", "debug", "trace", "config" );

>> 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.

I did. See config_file.hpp ib Boost.Test code base.

>> 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?

I mean that writing config files programmatically not necessary enough and 
should not part of this library.

Gennadiy