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

From: Thorsten Ottosen (nesotto_at_[hidden])
Date: 2004-03-19 06:51:11

Hi Vladimir,

Here are some thoughts based on reading the online docs.
I did not participate in the actual review, so I could easily
miss important design and implementation problems. So don't take my
comments too serious :-)

Section "Tutorial"
------------------------

1. I don't like that the example hardcodes strings.

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

3. why

po::store(po::parse_command_line(ac, av, desc), vm);

instead of

po::variables_map vm = parse_command_line( ... )

or

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

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.

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)

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?

7. In

po::value<int>(&opt)->default_value(10),

I would prefer

po::value<int>( opt, 10 ),

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

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

?.

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;

?

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

br

Thorsten