[boost] Re: Formal Review: Command Line & Config library

From: Vladimir Prus (ghost_at_[hidden])
Date: 2003-05-27 00:16:14

Hi Petr,

Petr Ko?m?d wrote:

> On Wednesday 21 of May 2003 13:59, Aleksey Gurtovoy wrote:
>> The formal review of Vladimir Prus' Command Line & Config
>> ("program_options") library starts today, May 21, and will continue until
>> Monday, June 2nd. I will be serving as review manager.
> 
> I take a deep look down to the code, not bothering with documentation nor
> actually testing it. I am looking at the submission packaged at 23.5.2003.

Thanks for the review!

> 1.
> I consider hardcoding of std:string throughout CL&CL to be a major design
> flaw. As it is, it's virtually nonportable to platforms/applications which
> could use unicode and/or platform using specific startup instead of C
> legacy main(). (Consider: Win32 GUI applications, Linux running unicode
> console, filesystems with uniceode file names/content, embedded devices).
> Why not using templates for strings with std:string as a default type?
> Some people are tied by API to slightly different string model, such as Qt
> or MFC strings (those mentioned are, or are not, depending on library
> mixture, compatible with std:string), with command line already filtered
> and passed down to some application object (no access to naked argv) and
> so on.

Yep. This question was raised already. I think that making templates out of 
all classes is *a* solution, but I'm not sure it's *the* solution. The
problem is that extensive template usage would prevent making library ---
either shared or static. I think that would be a big downside. 

It would probably be possible to use Unicode only at some specific layers 
(say, option_and_arguments class), and instantinate it for string and
wstring in library code. It's probably be even better to always work with
Unicode internally, and do conversion only on boundaries. 

> 2.
> Decoupling parsing from doing actual stream input would increase
> reusability. Spirit does parsing with iterators only, why not borrow some
> declarative idioms or actual code from it? 

Could you be a more specific? If you're talking about config_file then I
must say that previous version had something called 'line_iterator', which
was responsible for skipping comments/empty lines. Alas, it depended on
home-grown iterator adaptor, so it was dropped when boostifying code. 

I might revive it one day, using new iterator adaptors, but I fail to see
how this fill translate to benefit for users.

If command line is considered, then iterators don't really fit. 
- part of interface must be implemented in headers
- you can't use iterators to string for command line parsing, because
  separation of token into argv element is important for semantics.
  "a" and "b" as two separate argv elements is very different from
  "ab" or "a b"

> It would be more interesting to
> have an option/additional parser registration at compile time instead of
> at run-time since program options/configurations are rarely of dynamic
> nature.

Eeh... what do you mean by "interesting" ;-) I think that "rarely" is not
the same as "never". In early discussion it was asked if my library could
work with plugins. And plugins is an example of dynamic registraction:
you can load plugin, ask for options that it handles, and add those to
global set of options.

Or probably I'm missing your point? Can you give an example of what you mean
by dynamic registraction?

> 3.
> Also, I'd suggest to add some genericity on value retriving mechanism, at
> least at the interface level (e.g. replacing config_file with more
> abstract config_storage concept, consider: windows registry API, chip card
> reader API, shared memory, XML, ...).

In fact, that was planned from the very start. But... on a different layer.
The 'variables_map' class is polymorphic. One can define derived classes
which read from registry, or from shared memory. 

You could also extend config_file to read from registry, but that would have
some problems: registry is huge, and there's no need to read it all. 

> Actually, by it's interface a config_file class mixes together three
> different concepts: storage accessor concept (input stream with ownership
> management), content grammar definition concept (parser rulebase managed
> by add_option) and result-data-container-with-integrated-iterator concept

By the same argument, the 'istream_iterator<int>(cout)' mixes all those
concepts. 

> (with rather magical operator++, name() and value(), why not simple get()
> returning tuple? 

Because I believe 'name' and 'value' is a bit more better interface that
tuple (which fields don't have descriptive names).

> Or get() taking a key argument?)
> Such a mix of concepts in one class is confusing and prevents
> extensibility of all mechanisms. More, private implementation stuff should
> be not part of the interface in boost-quality code.

I'm sorry, but this is *very* unspecific. The class in question does not
have any implementation stuff in public sections. If you mean to say that
"all classes in boost namespace should use pimpl idiom, IMO", you probably
should have said that. Or did you mean something else?

> 4.
> Ability of storing configuration values back to persistant storage would
> be a natural outcome of correct solution to 3.

That's not so hard to add to 'variables_map' interface, actually.

> Well, what I like about CL&CL is the pretty idea of domain-specific option
> definition language, but I'd rather see a compile time solution of it. 

Could you explain what advantages will compile time solution have?

> I can't resist to say: "Spirit rules!" ;-)

I can't resist to say: "So what?" ;-) Just for example, do you think that
Spirit is of any help for 'variables_map' class? Or for
'options_description' class. Yep, probably you could change the library so
that Spirit is used for actualy parsing. But you still have to describe
options somehow.

> I believe if CL&CL is supposed to be an universal library, it should be
> redesigned adding some more versatility layers before its acceptation into
> boost.

Do you mean by "versatality levels" the things that you describe above, in
3) --- registry access, for example. That feature can be added very easily.
Or do you have more specific suggestions in mind?

Thanks,
Volodya