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

[Tobias Grosser]

On 05/02/2013 07:27 PM, Daniel Liew wrote:
> Thanks for taking the time to review. Please see my responses below.
>>> +  //Unhide -help-list if necessary
>>         ^ Space?                      ^ Point?
> Did you mean 'Point' as in 'what is the point?' or missing fullstop?

I meant 'fullstop'. Sorry for my english.

>>> +  // 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...

The later.

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

This sounds good.

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

Just run it over all your code and see which differences it proposes. 
Not all may be useful, but if there is no good reason to format it 
differently, I would go for clang-format's choice.

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

I don't have a strong opinion here. Do what you believe is better.

>> 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,
Tobias