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

[Tobias Grosser]

On 04/09/2013 01:09 PM, Daniel Liew wrote:
> I realised my naming of the default option category wasn't such a good
> idea (a category called "Default" is confusing) so I've amended my
> first patch so that the default category is called "General".
>
> The amended patch (001-amended.patch) is attached. I should also note
> that patch 001 needs to be applied before patch 002.

I like the general idea. I have no idea about the design of the option 
class, but gere some style comments:

> ---
>   include/llvm/Support/CommandLine.h |   50 ++++++++++--
>   lib/Support/CommandLine.cpp        |  150 +++++++++++++++++++++++++++++++++---
>   2 files changed, 185 insertions(+), 15 deletions(-)
>
> diff --git a/include/llvm/Support/CommandLine.h b/include/llvm/Support/CommandLine.h
> index 2e84d7b..6331096 100644
> --- a/include/llvm/Support/CommandLine.h
> +++ b/include/llvm/Support/CommandLine.h
> @@ -137,7 +137,24 @@ enum MiscFlags {               // Miscellaneous flags to adjust argument
>     Sink               = 0x04   // Should this cl::list eat all unknown options?
>   };
>
> +//===----------------------------------------------------------------------===//
> +// Option Category class
> +//
> +class OptionCategory {
> +private:
> +  const char* name;
> +  const char* description;

Variables start with uppercase letters.

> +public:
> +  OptionCategory(const char* _name, const char* _description=0) {

Variables start with uppercase letters.

No need for the underscore here. Just initialize the values in the 
initializer list.

> +    name=_name;
> +    description=_description;
> +  }
> +  const char* getName() { return name;}
                                          ^ Space

> +  const char* getDescription() { return description;}
> +};
>
> +//The general Option Category (used as default category)
      ^ Space
> +extern OptionCategory generalCategory;
>
>   //===----------------------------------------------------------------------===//
>   // Option Base class
> @@ -160,6 +177,9 @@ class Option {
>     // Out of line virtual function to provide home for the class.
>     virtual void anchor();
>
> +  //Register Option Category

Space between // and the first word.

> +  void registerCategory();
> +
>     int NumOccurrences;     // The number of times specified



> +  OptionCategory* category; //The Category this option belongs to

Start the variable name with an uppercase letter.


> +// cat - Specifiy the Option category for the command line argument to belong
> +// to

This seems to be an old doxygen style comment. We now do not repeat the 
structure name but just start with the comment.

> +
> +struct cat {
> +  OptionCategory& category;
> +  cat(OptionCategory& c) : category(c) {}

I am unsure about the position of the '&' sign. Can you verify what the 
style is in LLVM. (In general you can use clang-format to check your code.

> +
> +  template<class Opt>
> +  void apply(Opt &O) const { O.setCategory(category); }

Start with uppercase.


> diff --git a/lib/Support/CommandLine.cpp b/lib/Support/CommandLine.cpp
> index 560d7eb..c191f9c 100644
> --- a/lib/Support/CommandLine.cpp
> +++ b/lib/Support/CommandLine.cpp
> @@ -31,6 +31,7 @@
>   #include "llvm/Support/Path.h"
>   #include "llvm/Support/raw_ostream.h"
>   #include "llvm/Support/system_error.h"
> +#include <map>
>   #include <cerrno>
>   #include <cstdlib>

Please sort alphabetically. Also, are you sure you want to use a map and 
not one of the LLVM specific ADTs?

>   using namespace llvm;
> @@ -106,6 +107,17 @@ void Option::addArgument() {
>     MarkOptionsChanged();
>   }
>
> +// This collects the different option categories that have been registered

Missing point at the end.

> +typedef SmallPtrSet<OptionCategory*,16> OptionCatSet;
> +static ManagedStatic<OptionCatSet> registeredOptionCategories;
> +
> +//Initialise the general option category

      ^ Missing space.

> +OptionCategory llvm::cl::generalCategory("General options");
> +
> +
> +void Option::registerCategory() {
> +  registeredOptionCategories->insert(this->category);

Why do you need 'this->' here?

> +}
>
>   //===----------------------------------------------------------------------===//
>   // Basic, shared command line option processing machinery.
> @@ -1227,6 +1239,15 @@ class HelpPrinter {
>   public:
>     explicit HelpPrinter(bool showHidden) : ShowHidden(showHidden) {}
>
> +  typedef SmallVector<std::pair<const char *, Option*>,128> StrOptionPairVector;
> +
> +  virtual void printOptions(
> +    StrOptionPairVector& Opts, size_t MaxArgLen)
> +  {
> +    for (size_t i = 0, e = Opts.size(); i != e; ++i)
> +      Opts[i].second->printOptionInfo(MaxArgLen);
> +  }
> +
>     void operator=(bool Value) {
>       if (Value == false) return;
>
> @@ -1236,7 +1257,7 @@ public:
>       StringMap<Option*> OptMap;
>       GetOptionInfo(PositionalOpts, SinkOpts, OptMap);
>
> -    SmallVector<std::pair<const char *, Option*>, 128> Opts;
> +    StrOptionPairVector Opts;
>       sortOpts(OptMap, Opts, ShowHidden);
>
>       if (ProgramOverview)
> @@ -1267,8 +1288,7 @@ public:
>         MaxArgLen = std::max(MaxArgLen, Opts[i].second->getOptionWidth());
>
>       outs() << "OPTIONS:\n";
> -    for (size_t i = 0, e = Opts.size(); i != e; ++i)
> -      Opts[i].second->printOptionInfo(MaxArgLen);
> +    printOptions(Opts,MaxArgLen);
                          ^ Space
>
>       // Print any extra help the user has declared.
>       for (std::vector<const char *>::iterator I = MoreHelp->begin(),
> @@ -1280,6 +1300,88 @@ public:
>       exit(1);
>     }
>   };
> +
> +class CategorizedHelpPrinter : public HelpPrinter
> +{
> +public:
> +  explicit CategorizedHelpPrinter(bool showHidden) : HelpPrinter(showHidden) {}
> +
> +  //Helper function for printOptions()
> +  //It shall return true if "a" should be ordered before "b", false otherwise
        ^ Space
> +  static bool OptionCategoryCompare(OptionCategory* a,OptionCategory* b) {
                                        OptionCategory *A
> +    assert(strcmp(a->getName(),b->getName()) !=0 &&

> +  //Make sure we inherit our base class's operator=()
Space after //

> +  using HelpPrinter::operator=;
> +
> +  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 sort them alphabetically
> +    for (OptionCatSet::const_iterator
> +         i= registeredOptionCategories->begin(),

Use I as an uppercase letter.

> +         E= registeredOptionCategories->end();
> +         i != E; ++i)
> +    {

This should be on the same line as the )


> +      sortedCategories.push_back(*i);
> +    }
> +    assert(sortedCategories.size() > 0 && "No option categories registered!");
> +    std::sort(sortedCategories.begin(),
> +              sortedCategories.end(),
> +              OptionCategoryCompare);

Does this fit in fewer lines?

>   // Define the two HelpPrinter instances that are used to print out help, or
> @@ -1288,6 +1390,12 @@ public:
>   static HelpPrinter NormalPrinter(false);
>   static HelpPrinter HiddenPrinter(true);
>
> +// Define the two CategorizedHelpPrinter instances that are used to
> +// print out help-cat or help-cat-hidden with the
> +// options displayed in categories

Missing '.' at the end of the sentence. Also, please fill the 80 column 
limit.


I stopped here, but I hope this gives you an idea of some of the style 
problems that need to be fixed.

Tobias