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 ---
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
