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