[boost] Re: Formal review: program_options

From: Vladimir Prus (ghost_at_[hidden])
Date: 2003-06-03 00:57:03

Hi Beman,

Beman Dawes wrote:

> This review is based purely on reading the documentation. The code was not
> inspected and no tests were run. I also skim read some of the other review
> comments.
> 
> In general, I like the library and think that it should be accepted by
> Boost. 

Thanks!

> But there are a number of issues, and I have the feeling that
> careful revision based on review comments would pay big dividends.

That's my opinion, too. Some of the comments already made would make
interface much more clear.

> Design Issues
> =============
> 
> * As someone else already mentioned, cmdline and config_file might be
> easier to use by STL-experienced programmers if they more clearly modelled
> containers (with separate iterator types), or else followed the
> options_and_arguments usage, which returns a vector<> ref that can be
> iterated over. The current design seems to mix typical container
> operations with typical iterator operations.

Thanks for previous comment of Misha Bergal, I realize why interface of the
two classes might be confusing. OTOH, it's still unclear what new interface
might be. I can imagine:

  struct option {
        std::vector<std::string> value; 
        virtual ~option() {}
  };
  struct named_options : option {
        std::string name;
  };
  struct positional_option : option {
        int position;
  };

With option* as value_type of iterator. But this design looks not that good.
In effect, cmdline returns both named options and positional options, while
iterator has only one value_type. I could use boost::variant, but that's
extra dependency, and I'm not sure I'd like it.

Would it still be better to hide both classes from the public interface --
so that nobody would want to use them?

> * boost::program_options::options_and_arguments - the difference between
> the get_values() and get_all_values() members isn't clear. Not sure if
> this is just a doc issue, or there is an underlying design confusion.

I think it's a doc issue. Imagine two cases:

   prog --foo 1 2 --bar 3
   prog --foo 1 2 --bar 3 --foo 4

In first case, get_values("foo") will work --- returning ["1", "2"]. In the
second case, it will not work, because "--foo" occurs two times on the
command line, and implicitly merging values from differenc occurences is
not good. So, in the second case, one should call "get_all_values", which
will return [["1", "2"], "4"]

> * wchar_t and UDT-char support. These are probably important long-term
> goals, but IMO should be deferred until more experience has built up with
> the basic narrow-char design.

Agree.

> Questions
> =========
> 
> * Does a programmer have to be aware of a distinction between command line
> args and config file args in normal usage? (This may be a documentation
> issue rather than a design issue. Perhaps "Design overview" section (1)
> dealing with cmdline and config_file needs an added sentence like "Unless
> a user wishes to explicitly control parsing, the use of these parser
> classes is hidden from the programmer.")

My intention is that typically, programmer only calls 'parse_command_line'
and 'parse_config_file' functions, and indeed does not work with parser
classes. I though their use makes sense --- in one case some person wanted
absolutely minimal command line parser --- but now that everybody seem to
care only about options_description interface, those classes may be hidden.

> * Are common forms of command line indirection to a config file
> supported?  Like @pathname, for example?  [Later... it really doesn't look
> like cmdline does indirection. If not, that's a serious oversight, IMO.
> Easy to fix, however.]

Not directly. The Wave preprocessor implements response file on top of
program_options. Essentially, there's special option "config-file" (which
can be specified as "@filename"). After parsing, if this option is present,
the named file is opened and parsed too). I'm planning to provide this
functionality out-of-the-box.

> * Can indirection be recursive? (@file1 contains an indirection like
> @file2.) If so, are cycles detected?

The above scheme is quite simple and does not handle recursion.

> * How do you code a comment in a config file? (Comments in config files
> are really important - this feature needs to be added if not already
> supported.)

The '#' character starts a comment.

> * Is there a function or class which does the argc/argv replacement-trick
> for command line indirection? It is so easy (one #include + one line of
> code) even for existing programs that it might be valuable as a standalone
> feature even if built-in to the cmdline parser itself.

That's very interesting idea! Two caveats, though:
1. The function for replacement should operate on vector<string>, since you
can't change argv, in general.
2. You've got to do some parsing of file. For example, if it contains

  -c foo

then you'll have to add *two* elements to argv. If you add one, it will be
most likely parsed as option "-c" with value " foo" (note the extra space).

> Minor Issues and Quibbles
> =========================
> 
> * boost::program_options::options_and_arguments member names: I'd prefer
> just plain "value", "values", and "all_values". The "get_" adds little and
> isn't usual Boost or Standard Library practice.

Point taken. Will rename.

> * The C++ standard, 3.6.1, specifies the declaration of main as "int
> main(int argc, char* argv[])". Your options_description.html,
> custom_syntax.html, and cmdline.html examples uses "int main(int ac, const
> char **av)", and that will cause problems with some compilers, IIRC.

Oops! I always though the standard has "const" there... I'll check again and
make adjustments.

> * Is there a style for +foo -bar style arguments where "+" yields a value
> of true and "-" yields a value of false?

This style was not considered very common. For that reason it is supported
via custom parser only -- but it's easy to write such a parser.

> Docs
> ====
> 
> * Need work to make them more consistent with usual Boost practice. Logo,
> etc. I tried very hard to get to like the doc style, but just plain feel
> it isn't as good as the traditional Boost approaches.

IMO, there's one problem with docs. I knew from the start, that
Doxygen-generated docs tend to be unusable, because they document member
functions with comments like "Constructor." and omit overview altogether.
I've tried to avoid this, but did not really succeeded.

As I've indicated, the plan is to convert the documentation to BoostBook.

> * Grammar and English usage are a bit awkward in places. 

I'm not a native speaker :-(

> I'll volunteer to do a pass over the docs once in final form.

Thanks a lot!

> 
> * The "simple usage" link points to a section entitled "Options
> description", which seems a misnomer since what is there is in fact a nice
> example.
> 
> * The options_description.html example would be much improved by showing
> several command line invocations with different arguments together with
> the output that each produced.

Agreed. Already in todo.

> * At first glance the options_description.html example's try block
> contains code that I would have guessed should be outside the try block.
> Is this ever explained?

I usually wrap the entire "main" function in try block. Does this cause any
confusion here?

> * Acknowledgements section missing.

Right. I have a lot of persons in my notes, and will acknoledge their input
when reworking docs.

> * Some of the filenames are too long:
> classboost_1_1program__options_1_1cmdline.html

Oh... doxygen. Hopefully BoostBook will do better.

> 
> * classboost_1_1program__options_1_1cmdline.html mentions operator++
> several times, but there is no public member of that name, and no public
> members which might return an iterator.

Right. The comment on operator++ was ordinary one, while it should be
Doxygen comment.

> * classboost_1_1program__options_1_1cmdline.html/Member Function
> Documentation/Parameters refers to "'next' member function", but there
> isn't any member function named 'next'.

Fixed, thanks.

> * classboost_1_1program__options_1_1options__and__arguments.html says that
> the "unsigned count(const std::string & name) member "Checks if option is
> present". Should that be "Returns the number of options present named
> 'name'"?

That's better. Changed.

> * classboost_1_1program__options_1_1cmdline.html style_t description would
> be a lot clearer if given in a table with columns name and meaning, and
> one row per style. There isn't any need to see the actual values assigned
> each style.

Noted. I'll try to do something about it.

Thanks,
Volodya