Re: [boost] Re: [program_options] post-review version

From: Vladimir Prus (ghost_at_[hidden])
Date: 2004-03-19 07:43:26

Hi Thorsten,

> Section "Tutorial"
> ------------------------
>
> 1. I don't like that the example hardcodes strings.

Could you explain? 

> 2. each line should have a comment about what it does.

Actually, that would duplicate explanations that are already in the tutorial. 
Don't you think?

> 3. why
>
> po::store(po::parse_command_line(ac, av, desc), vm);
>
> instead of
>
> po::variables_map vm = parse_command_line( ... )

Technically, this can be done by providing an operator= which takes an 
instance of "parsed_options".  But you'd still need 'store' for the second 
source.

> or
>
> vm.store( ... ) ? It would make it clearer what is stored in what.

I figured out that 'store' does not need to be member, so made it non-member. 
There are no other reasons.

> 4. po::variables_map is probably a std::map, right? I think that the way to
>     check if an argument is a bit strange, ie,
>
>     if (vm.count("help"))
>
> should be
>
>    if( vm.has_argument( "help" ) )
>
> or something.

The first approach is quite idiomatic, IMO. If there's enough user pressure, 
'has_option' can always be added.

> 5. Why is it necessary to remember the type of compresion argument? ie,
> could
>
>     cout << "Compression level was set to "
>            << vm["compression"].as<int>() << ".\n";
>
> not be written as
>
>     cout << "Compression level was set to "
>            << vm["compression"] << ".\n";
>
> ? (if eg. a boost::variant is returned, it would just write the int)

Hmm... I did not know about operator<< for boost::variant until now. But in 
general, you do need to know about exact type to do any processing of the 
value.

> 6. Consider
>
> desc.add_options()
>     ("help", "produce help message")
>
> why don't you have an add_options that take two and three arguments, så the
> first empty ones can be removed?

"The first empty parenthethis"? This idea did not occur to me. I think this is 
doable, though again, I'd like to get more opinions.

> 7. In
>
> po::value<int>(&opt)->default_value(10),
>
> I would prefer
>
> po::value<int>( opt, 10 ),

But then 

   po::value<int>( 10 ),

comes as obvious addition, too? I think that's rather good idea.

>  8.I don't find this too readable:
>
> po::store(po::command_line_parser(ac, av).
>           options(desc).positional(p).run(), vm);
>
> compared to
>
> po::command_line_parser parser(ac, av);
> parser.set_options( desc );
> parser.set_positional_options( p );
> vm.store( parser.run() );

Opinions differ ;-) I prefer the first one for compactness -- in fact I'm just 
emulating named parameters. Maybe when we have named parameters library in 
Boost the syntax can change.

> 9. Could this way of specifying composing()/default_value() be of any use
>
> option += option<int>( "xx", "xx msg" ), option_with_default<int>( ""yy",
> var_name, 0, "yy msg" ),
>                 option_with_compose< vector<string> >( "zz", "zz msg" );
>
> ?.

I think that syntantically, it requires the same number (or even larger) 
number of characters. Besides, what if you want to specified both default 
value and 'composing' flags? options_with_default_and_compose looks too long.

> 10. This
>
> po::options_description cmdline_options;
> cmdline_options.add(generic).add(config).add(hidden);
>
> might be
>
> po::options_description cmdline_options;
> cmdline_options += generic, config, hidden;
>
> ?

Interesting! I just found some-years-old code of mine which provides support 
for such initialization. It worked for vectors, like:

   v << 1, 2, 3, 4, 5, 6;

but can be easily used for +=, too. I'm just not sure this is justified here.

> 11.
>
> Have you compare with the functionality of other program option libraries?
> I mean, it is always nice to know that this is just the best around :-)

I've looked at several other libraries during development and still have the 
list somewhere. Unfortunately, no comparison is present in docs. In short, 
most of the libraries I've seen hardcode the type of supported options. I did 
not seen support for several configuration sources, either.

Thanks for your comments.

- Volodya