Hi, This is not a review of the supplied library. I am not gonna discuss docs even though they are scarce. I almost don't mention implementation/code/ testing. I just want to express my opinion on design of the library.
Following list is not ordered by importance nor any other order. It just list of my points: 1. Terminology Terminology chosen by author is confusing to me. In my understanding: term "parameter" - originated from 'formal parameter", formal description of the expected value term "option" - special case of parameter that describe parameters with boolean values term "argument" - originated from 'actual argument', actual value passed as expected value The way these terms are used in library does not seems to follow above definitions. 2. Layered design. The task in hand indeed ask for the "layered design". But design presented in library does not come close to what I understand under this term. Let's see what are the layers and their purpose a. First layer cmdline/config_file. It allows parsing argc/argv producing list of pairs of string. It does not know anything about description correct types and so on. The question is: why would I want in practice to use this layer standalone, instead of program options with all types defaulted to string? Why not hide this class into implementation details? b. Second layer declared in design is class options_and_arguments. What this class adds to the cmdline functionality (other than unnecessary copy of huge object - not that performance is that important, but still) and why it could not be implementation detail of the class cmdline unclear to me. c. Third level declared is program options. Looks like the only usable layer to me. d. Finally variable_map - third place where actual argument are stored (Number of places where actual value is stored looks redundant). Why would I want to use this class if I need only CLA parsing unclear to me. Multi-source case discussed below. So, from what I view there is only one layer presented in library design that fits the real needs. While IMO it should be several. 3. Variety of different name/value syntaxes support Library struggle to support most used syntaxes for the CLA processing. I am not gonna say that any specific style is missing. Let's better see how this support is designed. class cmdline is responsible for all syntax related tasks. The only way to select non-default style is to provide some bitmask of desired styles in constructor (BTW, I did not see an example of usage custom style with variable map - is it possible?). This has many problems: a. Limit number of supported styles die to bitmap limitation b. Force same style for all parameters in command line (I could not define /h --my_long_param) c. This kind of design when one class is responsible for many different styles *always* leeds to macaroni style unreadable code. See on cmdline implementation: endless list of if, else, and switch. And with only minimal number of styles (IMO) supported. How it will look in a year when author adopt all requests from users I could only imagine. IMO this is not a kind of style we want to promote. 4. Parameter definition. First of all I want to remark that there are in fact to ways to define a parameters in reviewed library. And both are not what I am looking. Program parameter definition is not repetitive task. We do it once, somewhere in program main most probably. As a developer or maintainer looking on unfamiliar code in need to see what are the parameters it supports, I want to read parameters descriptions like a poem, without need to solve a puzzle or guess implicit rules. From this rationale I believe that primary interface for parameter definition should be as verbose as possible. You may supply shortcuts for lazy programmer, but only as a second option. You may laugh, but even after several ours I spent with library I still could not figure out what second parameter in ("name", "", "") is. Parameter description should be clear to anybody who never read your docs. 5. Code organization Library implemented as an external library. IMO it's unacceptable for such basic component like CLA processing. test programs just a small part among all programs, but I still got numerous complains about absence of the inline version. Related topic is dependencies: IMO library implementation use too many dependencies, which became important once you use it inline. Though it's subtle point. 6. cmdline - container or iterator? Looking on class cmdline one will be puzzled: what king of design if follow? Is it container of iterator? Seems like both. If I remember correctly this is how some ancient pre-standard, or RW containers were implemented. I don't believe this is kind of design we want to promote. 7. Overblown interfaces In my taste interface to many major classes unreasonably overblown, which is not a kind of practice we should promote. See cmdline and option_description interface for example. 8. Separators handling Library silently assign special meaning to space symbol. The way parsing is designed there is no way to use parameter names with spaces inside. 9. Multi-pass parsing support I did not find any support for the multi-pass parsing. What I mean is that every parser parse CLAs and eats what it understand and leave the rest. It's important feature that require special support inside the library 10 Errors notification: usage of exceptions I believe that there are 2 cases when we should prefer to use exceptions instead of return value to notify about the error: a. function already have a return value b. We expect that user will want to propagate the error and handle it in scope different from one above. In case if a. we could return the value b. we assume that user would want to catch the error immediately in place of call c. user in majority of cases does not bother about type of error I do not see any reason to use exceptions for errors notification. IOW why user would need to write code like this: try { foo(); catch( std::exception const& ex ) { cout << ex.what(); } I believe that with CLA parsing we are namely in later case. Library should be able to provide user with interface that does not throw on error, but instead provide error return value and access to the error message. 11 Alternative search algorithms In a first statement of documentation author defines "program options" as name/value pairs. IMO this is way too limiting. For once we need to support positional parameters. But in general IMO it should be possible to support almost arbitrary parameters identification. Let say I want to pass list of Points,Lines and Circles as a command line arguments. Point syntax is (N1,N2). Line syntax is (Point1, Point2). Circle syntax is (Point, N). I expect CLA parser to be able to parse the input. Moreover in many interfaces library assumes existence on long name, short name, which may not be the case even with name based parameter identification. 12. Usage of pointers The decision to use pointers instead of references for const string and value location passing looks incorrect to me. 13 Config file format config file class supports basically only one style of parameter definition. Also in my opinion config file is more basic concept. It may be used for completely different purposes. We need more than one layer to correctly model config file. Accordingly presented design is unacceptable IMO. 14. Multiple sources support. Library declare support for multiple sources of program parameters. But IMO it is based on incorrect basic assumption that same parameter in different sources has the same name. Unfortunately I believe that it almost never the case. Each kind of source has different naming conventions and formatting rules. For example Boost.Test log level parameter would look like : -log_level - in command line BOOST_TEST_LOG_LEVEL - in environment boost/test/log/level - in registry boost::test::log::level - in configuration file To support multiple sources we need also deal with multiple conflict resolving algorithms. Presented design accordingly is unacceptable to me. 15. External parsers support. Library only supports one extra external parser per parsing. That may be very limiting in case if I want to implement SAX like, callback based CLA parsing. I may be missing something but all in all supplied library design looks *completely unacceptable* IMO. Other reviewers seems to be pretty happy with it. So be it. I have already implementation of CLA parsing that fits to my requirements (it's in vault area). It's not finished yet. I may submit it once I am done. Regards, Gennadiy. _______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost