"Marcin Kalicinski" <kalita@poczta.onet.pl> wrote in message news:e27vp4$o1p$1@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