[boost] Formal review: program_options

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 
comments.

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!

--Beman

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.

* 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.

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

* 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 
supported.)

* 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?

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.

* 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 
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.

* 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?

* Acknowledgements section missing.

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

* 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 
'name'"?

* 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.

--- end ---