From: Marcin Kalicinski (kalita_at_[hidden])
Date: 2006-04-20 08:45:27
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:
Now suppose you have a component which is only interested in option1 and
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
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
> 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
Can you beat that simplicity (4 lines of code)? :
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?
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk