From: Beman Dawes (bdawes_at_[hidden])
Date: 2003-06-02 11:37:11
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
In general, I like the library and think that it should be accepted by
Boost. But there are a number of issues, and I have the feeling that
careful revision based on review comments would pay big dividends.
Thanks to Volodya for such a useful submission!
* 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.
* 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.
* 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.
* 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.")
* 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.]
* Can indirection be recursive? (@file1 contains an indirection like
@file2.) If so, are cycles detected?
* 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
* 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.
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.
* 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.
* Is there a style for +foo -bar style arguments where "+" yields a value
of true and "-" yields a value of false?
* 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.
* Grammar and English usage are a bit awkward in places. I'll volunteer to
do a pass over the docs once in final form.
* The "simple usage" link points to a section entitled "Options
description", which seems a misnomer since what is there is in fact a nice
* 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.
* 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
* Acknowledgements section missing.
* Some of the filenames are too long:
* 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.
* 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'.
* 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
* 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
--- end ---
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk