[boost] CLA/Config file library opinion

From: Gennadiy Rozental (gennadiy.rozental_at_[hidden])
Date: 2003-05-26 02:54:41

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.