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

[Andrew Trick]

Daniel,

I reviewed your most recent patch (MOREFIXES) and your getRegisteredOptions patch. They both look very nice.

I have one suggestion. For normal (non-hidden) help, we may have empty categories, which looks odd. Can you just suppress printing the category in that case?

Also, would you mind adding a couple simple tests to CommandLineTest.cpp?

Otherwise, it's ready to commit. Will you need me to check it in?

-Andy

On May 3, 2013, at 6:58 AM, Daniel Liew <daniel.liew at imperial.ac.uk> wrote:

> I've tried to address the issues you mentioned Tobias. The patch is
> attached.
> 
> Have you been able to look at the patch for the second feature (
> cl::getRegisteredOptions() function ) I want to add?
> 
> Thanks,
> Dan.
> 
> On 03/05/13 13:36, Tobias Grosser wrote:
>> On 05/02/2013 10:14 PM, Daniel Liew wrote:
>>> Oh dear there was a mistake in the first patch I gave you. I forgot to
>>> remove comments relating to -help-cat and -help-cat-hidden (which was
>>> part my old implementation)
>>> 
>>> I've attached the amended patch and also the patch for
>>> cl::getRegisteredOptions() feature (now has documentation as part of the
>>> patch and line endings cleaned up)
>>> 
>>> Sorry for the confusion.
>> 
>> Hi Daniel,
>> 
>> thanks for addressing my comments. The new patch looks already very
>> nice. I tested it and it continues to work as expected. I have a last
>> set of minor stylistic comments, but the patch seems to be OK after
>> those have been addressed.  Maybe you can repost the patch with the last
>> fixes, such that Andrew can have another look.
>> 
>> Thanks,
>> Tobias
>> 
>>> +Grouping options into categories
>>> +--------------------------------
>>> +
>>> +If our program has a large number of options it may become difficult
>>> for users
>>> +of your tool to navigate the output of ``-help``. To alleviate this
>>> problem we
>> ... of our tool ...
>> 
>> Either address the reader using 'you' or 'us'. Mixing those forms in a
>> single sentence seems confusing.
>> 
>>> +can declare option categories by statically declaring a
>>> `cl::OptionCategory`_
>>> +and then place our options into these categories using the `cl::cat`_
>>> option
>>> +attribute.
>>> +
>>> +.. code-block:: c++
>>> +
>>> +  cl::OptionCategory StageSelectionCat("Stage Selection Options",
>>> +                                       "These control which stages
>>> are run.");
>>> +
>>> +  cl::opt<bool> preprocessor("E",cl::desc("Run preprocessor stage."),
>>> +                             cl::cat(StageSelectionCat));
>>> +
>>> +  cl::opt<bool> noLink("c",cl::desc("Run all stages except linking."),
>>> +                       cl::cat(StageSelectionCat));
>>> +
>> 
>> I would call the previous options PreProcessor and NoLink according to:
>> http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
>> 
>> 
>>> Option Modifiers
>>> ----------------
>>> 
>>> @@ -1385,6 +1438,27 @@ For example:
>>> 
>>>   cl::extrahelp("\nADDITIONAL HELP:\n\n  This is the extra help\n");
>>> 
>>> +.. _cl::OptionCategory:
>>> +
>>> +The ``cl::OptionCategory`` class
>>> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>> +
>>> +The ``cl::OptionCategory`` class is a simple class for declaring
>>> +Option categories.
>>> +
>>> +.. code-block:: c++
>>> +
>>> +  namespace cl {
>>> +    class OptionCategory;
>>> +  }
>>> +
>>> +A option category must have a name and optionally a description which
>>> are passed
>>> +to the constructor as ``const char*``.
>>> +
>>> +Note that declaring an option category before parsing options(e.g.
>>> statically)
>>                                                                ^ space?
>> 
>> I would add a space between options and '('.
>> 
>>> //===----------------------------------------------------------------------===//
>>> 
>>> // Basic, shared command line option processing machinery.
>>> @@ -528,6 +540,7 @@ static void ExpandResponseFiles(unsigned argc,
>>> const char*const* argv,
>>>   }
>>> }
>>> 
>>> +
>> 
>> You add an unnecessary newline here.
>> 
>>> static cl::opt<bool>
>>> PrintOptions("print-options",
>>> @@ -1306,6 +1447,24 @@ PrintAllOptions("print-all-options",
>>>                 cl::desc("Print all option values after command line
>>> parsing"),
>>>                 cl::Hidden, cl::init(false));
>>> 
>>> +void HelpPrinterWrapper::operator=(bool Value) {
>>> +  if (Value == false)
>>> +    return;
>>> +
>>> +  // Decide which printer to invoke. If more than one option category is
>>> +  // registered then it is useful to show the categorized help
>>> instead of
>>> +  // uncategorized help.
>>> +  if (RegisteredOptionCategories->size() > 1) {
>>> +    // Using categorized printer so should unhide -help-list so user
>>> can have
>>                                   ^^^^ This 'so' seems useless and a
>> little confusing.
>> 
>> Cheers,
>> Tobi
> 
> <0001-MOREFIXES-Support-command-line-option-categories.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130503/1c663029/attachment.html>