[boost] Re: CLA/Config file library opinion

From: Gennadiy Rozental (gennadiy.rozental_at_[hidden])
Date: 2003-05-27 04:36:33

"Vladimir Prus" <ghost_at_[hidden]> wrote in message
news:basnho$kmt$1_at_main.gmane.org...
> Hi Gennadiy,
>
> Gennadiy Rozental wrote:
>
> >   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.
>
> Thanks for taking the time to comment!
> I actually expected critical opinion from you, and while I don't think I
can
> change your mind, it's still a good idea to answer your concerns for the
> benefit of other reviewers.
>
> But two preliminary points:
>
> First, it's very sad that we both put time in different libraries for the
> same task. I'm not sure we could have come to a conclusion, but IIRC,
> you've decided to write your own after looking at 'cmdline' class from my
> library only --- the most basic and internal one --- and not at anything
> else. Had you drop an email saying what you don't like, we'd probably have
> a more pleasant situation.

I did not change my opinion after review your preliminary submission. It's
difficult to expect we could find a consensus, when I disagree with almost
every single one design decision in your library.

> > 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.
>
> I'm not claiming the definitions are perfect, but I do claim it's not 100%
> broken.

Try to provide a definition for term 'option' the way it's used in your
library. You will immediately find conflict.

> > 2. Layered design.

You would not argue specific subpoints of this issue. IMO All of my concerns
are unfortunate consequences of the way you designed your library. IMO there
should be one typed storage for the parsed arguments (instead of 3 like you
have) linked with formal parameters description. Plus second layer managing
composition of parameter originated by different sources.

> > 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?).
>
> Hmm... custom style has nothing to do with variables map. You specify
custom
> style to command line parser, and variables map is created from results of
> parsing. As such, it's completely independent.

The question was: could I use, for example, / as a separator and retrieve
values from variable_map

> > This has many
> > problems:
> >    a. Limit number of supported styles die to bitmap limitation
>
> The number of directly supported styles will always be limited. For all
> others, users will be compelled to write they code --be it a function,
> or a policy.

It's only limited in your design.

> >    b. Force same style for all parameters in command line (I could not
> > define /h --my_long_param)
>
> Do you need it in practice?

Why not. Maybe different modules require parameters in different style.

> >    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.
>
> First, it would not adopt "all styles". In already supports a fair number
of
> them, and more elaborate styles will be supported by custom parsers.
>
> Secondly, "macaroni", as you put it here, sounds a bit... unconstructive.
> You don't even try to name the functions that you find poorly written.

Whatever way you name it, it will always be if this style is on if that
style is on and so on. I do not need to  look onto specific implementation-
it will always be this way. Also IIRC there some conflicting styles that are
unreasonably difficult to implement within bounds of one parser - and no
need to do so.

> > 4. Parameter definition.

> I think, that if you don't think maintenance programmer will ever read
docs,
> you can help him in a simpler way
>
>   desc.add_options()
>        // first argument is options name
>        // second argument is parameter name
>        // third argument is description
>        ("output", "file", "output file")
>        ....
>        ;
>
> This means that one comment line is added for first argument, and then you
> can very conveniently specify option name.

Poor maintenance programmer looked on this code and spread hands puzzled:
What is option name and what is parameter name???? Which one should I use to
identify argument? Where the value assigned? What is default value?

> > 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.
>
> I subscribe to the view in Boost coding guidelines by David Abrahams,
where
> he says that inlining is either second or third (don't remember exactly)
> most misused feature in C++.
> If a library can be implemented externally, it should. Linking to a
library
> is minor problem. Inlining/templating will only
> - add compilation time. E.g. profile build of a simple program which uses
> BGL takes several minutes for me. Yes, program_options is not as large,
but
> what if you add 20 small libraries...

Execution Monitor incomparably smaller. Weren't you the one who requested
inline version of it?
In general I may also be satisfied with external library. But I am sure that
there are some people that don't. Also an ability to make external library
is only consequence of your design. Once you move to mode generic solution
it disappear immediately.

> - add distribution size. E.g. if program_options is accepted in Boost,
next

It's really minor point. We use CLA parsing only once in application.

> > 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.
>
> Alas, this comment seems non-constructive for me. I don't think that
> the question is what kind of design should be promoted. What are the
> problems with the current design? Can you list some interesting things
that
> would be possible if config_file were an iterator? What would be its
> value_type? And what will operator++ do on error?

I do not want to play games with examples in this case - it's too obvious:
it's bad idea to mix 2 different concepts in one class. No wonder it's not
used any more. And let me repeat: I do not think  we should promote this
kind of design (even, as you think, you were able for now to avoid problems
with it).

> > 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.
>
> I can only say that my taste differs. The level of cohesion of
> option_description seems pretty high for me.

class cmdline access methods:
  option_name()
  option_index()
  raw_option_name();
  option_value();
  option_values();
  argument();
  arguments();

You obviously struggle  from need to provide on one hand container
interface, on the other iterator one. Plus instead of N methods returning
properties of option there should be one that returns foe example 'option'
structure.

> > 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.
>
> Right -- in command line. Do you need it there?

I did not get you answer. What I need is an ability to write like this:

my_prog --service name=>Repository ...

 Here parameter name is service name, separator is string "=>", value is
'Repository'.

> > 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
>
> Could you please clarify why it's important feature? The proposed library
> allows to combine options_description instances, parse the command line,
> and then pass the result to interested modules.

It's important for me as Boost.Test developer ;-). I need to swallow
Boost.Test parameters and pass the rest to the user program. I also met the
same logic several times in past (with Visibroker for example). And you
methods would not work here.

> > 10 Errors notification: usage of exceptions

> I believe that exceptions are more convenient means. The handling of
options
> is not one line. First you declare options, and can make error there. Then
> you parse command line and can have errors there. Later, you might want to
> parse config file, which can be inaccessible, or have invalid syntax, or
> something else. So, in example above, you have several function invocation
> in 'try' block. If you're use error codes, you'd have to check it after
> calling each function.

No. I would not. In majority of the cases I do not bother whether any
intermediate step fails, parsing or validation fails. Whatever happened
during CLA declaration, parsing and validation I just want to get a
notification about an error during CLA parsing procedure and have and access
to the error message. IOW it would look like this:

  cla::parser p;
  p << parameter<int>( "name" )
....
  if( ! p.parse(argc,argv) ) {
      cout << p.error_msg() << endl;
      p.usage();
      return 1;
  }

> > 11 Alternative search algorithms

> I've expressed this concern some time ago: I feel this example is either
> artificial, or is not directly related to command line parsing. If you
want
> to parse arbitrary syntax, you need a parser.

Exactly. I need flexible enough parser that could accommodate practically
any parsing. And I argue that it's not difficult to achieve.

> >    Moreover in many interfaces library assumes existence on long name,
> >    short
> > name, which may not be the case even with name based parameter
> > identification.
>
> I don't understand what you mean here.

I may not have long name, but only short one, like with old getopt had.
I may not have short name but only long one, like the style I am personally
prefer.

Your interface is too rigid, and could not accommodate these styles.

> > 12. Usage of pointers
> > The decision to use pointers instead of references for const string and
> > value location passing looks incorrect to me.
>
> Can you clarify what "const string". Why the decision is incorrect?

Why T* is used instead of T&?
Why char const* is used instead of std::string const&?

> > 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 don't understand what you mean. Could you clarify?

I do not want to store parsed values inside the parser at all. Instead I
want to register for every parameter callback function.

>
> Thanks,
> Volodya

Gennadiy.