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

From: Petr Koèmíd (pkocmid_at_[hidden])
Date: 2003-05-26 20:47:45

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.

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.

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? 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.

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, ...). 
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 (with rather 
magical operator++, name() and value(), why not simple get() returning tuple? 
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. 

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

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. I 
can't resist to say: "Spirit rules!" ;-)

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. 

Petr Kocmid