[PATCH] Extra Command Line library functionality (option grouping and runtime inspection/modification)

[Daniel Liew]

Thanks for taking the time to review. Please see my responses below.
On 02/05/13 17:39, Tobias Grosser wrote:
> On 04/22/2013 04:03 PM, Daniel Liew wrote:
>> On 22/04/13 11:02, Daniel Liew wrote:
>>
>> I tweaked the implementation and documentation slightly. The new version
>> of the patches are attached.
> 
> I just applied the patches and looked into them.
> 
> 
> == General comments ==
> 
> 1) Trailing white spaces
> 
> Your patches contain a large number of trailing white spaces. Please
> remove them.
Oops. I'll fix that.

> 2) Compile time warnings
> 
> I first noticed a couple of compile time warnings when compiling with a
> recent version of clang.
> 
> lib/Support/CommandLine.cpp:1241:7: warning: '<anonymous>::HelpPrinter'
> has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
> class HelpPrinter {
>       ^
> lib/Support/CommandLine.cpp:1309:7: warning:
> '<anonymous>::CategorizedHelpPrinter' has virtual functions but
> non-virtual destructor [-Wnon-virtual-dtor]
> class CategorizedHelpPrinter : public HelpPrinter
>       ^
> lib/Support/CommandLine.cpp:1423:1: warning: extra ';' outside of a
> function is a C++11 extension [-Wc++11-extra-semi]

I'll add virtual destructors. I'm not sure how that stray semi-colon
ended up there. I'll remove it.

> 3) Please merge the patches for documentation and code changes
Ok.


> == Option grouping ==
> 
> The patch compiles fine and does, as expected, not alter the default
> output of '-help'. I added now a new option group to Polly using the new
> option category syntax. When loading the Polly module the options are
> now grouped and the Polly options are shown in a separate group. For
> this test case the patch works as expected.
> 
> Looking at the general implementation of the patch, the changes seem
> reasonable to me. I only have several style, typo and dead code
> comments, but nothing serious.
> 
>> From 616e1326a6131357060e4918e4adc437c86ef908 Mon Sep 17 00:00:00 2001
>> From: Dan Liew <daniel.liew at imperial.ac.uk>
>> Date: Mon, 8 Apr 2013 23:32:21 +0100
>> Subject: [PATCH 1/5] Added support for declaring Option categories and
>> the
>>  ability to put declared options into categories.  In
>>  addition categorized help printers have been added.
> 
> The subject of the commit message if better to read if you keep it
> short. I would just call it 'Support command line option categories'.

Ok.


>> +  //Unhide -help-list if necessary
>        ^ Space?                      ^ Point?
Did you mean 'Point' as in 'what is the point?' or missing fullstop?



> You may want to mention that this function uses an alphabetic
> (lexicographic) order to sort the options.
Okay I will add that.


>> +  static bool OptionCategoryCompare(OptionCategory *A,OptionCategory
>> *B) {
>                                                          ^ Space
>> +    assert(strcmp(A->getName(),B->getName()) !=0 &&
>                                                   ^Space
>> +           "Duplicate option categories");
>> +    return strcmp(A->getName(),B->getName()) < 0 ;
> 
> Instead of calling strcmp twice, you may want to call it once, store it
> in a variable and compare against this variable.
Seems sensible. I'll change that.

> 
>> +  // Make sure we inherit our base class's operator=()
>> +  using HelpPrinter::operator=;
>> +protected:
>> +  virtual void printOptions(StrOptionPairVector &Opts, size_t
>> MaxArgLen) {
>> +    std::vector<OptionCategory*> SortedCategories;
>> +    std::map<OptionCategory*,std::vector<Option*> > CategorizedOptions;
>> +
>> +    // Find the different option groups and put into vector ready for
>> sorting.
> 
> Thi sentence sounds grammatically a little uncommon.

I'm not sure which sentence you are referring to "Make sure.." or "Find
the different...".  I'll assume that later, would prefer it to be
phrased as...

"Collect registered option groups in a vector in preparation for sorting."?

>> +    for (OptionCatSet::const_iterator
>> +         I = RegisteredOptionCategories->begin(),
>> +         E = RegisteredOptionCategories->end();
>> +         I != E; ++I)
>> +      SortedCategories.push_back(*I);
> 
> I propose clang-format to format this loop.

I'll see what clang-format spits out. Was it just this loop above you
are concerned about?


> The argument 'ToUnhide' seems to be unused?

Oops, yes that was left over from me trying to decide how the -help-list
option should be automatically unhidden.

I ended up putting the behaviour in a function unHideHelpListOption
cl::ParseCommandLineOptions() instead of the HelpPrinterWrapper class.

Maybe it would be cleaner if I put that behaviour back into the
HelpPrinterWrapper class (but without the ToUnHide argument)?


> Can you add a short explanation what uncategorized/categorized help
> printers are? Basically that this is a help printer which changes
> behavior depending on the existance of more than one option category.

I'll add something like that.

Thanks,
Dan.