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

[Daniel Liew]

Attached is my patch for adding command line option categories with the
documentation and with fixes to the style issues you previously mentioned.

On 02/05/13 18:36, Tobias Grosser wrote:
> 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
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Support-command-line-option-categories.patch
Type: text/x-patch
Size: 19008 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130502/bedf9064/attachment.bin>