[PATCH] Extra Command Line library functionality (option grouping and runtime inspection/modification)
Tobias Grosser
tobias at grosser.es
Thu May 2 09:39:27 PDT 2013
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.
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]
3) Please merge the patches for documentation and code changes
I prefer to have the documentation changes and the actual code changes
in the same commit.
4) Independent reviews for independent patches
Patch reviews are easier if we keep them small. The "Added table of
contents declaration in CommandLine Library documentation." patch is
trivial to review and should be committed first. I did this in r180919.
The other two features getRegisteredOptions() and the option grouping
seem to be independent. I will first review the option grouping.
== 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'.
> If only one option category is declared (cl::GeneralCategory is always declared)
> then the behaviour of -help and -help-hidden produces uncategorized help output.
>
> If two or more option categories are declared then the behaviour of-help and
> -help-hidden automatically switches to categorized help output and the
> -help-list option becomes visible which shows uncategorised help output. Note
> -help-list-hidden is also available too and just like -help-hidden it is hidden.
> +++ b/include/llvm/Support/CommandLine.h
> + const char *ArgStr; // The argument string itself (ex: "help", "o")
> + const char *HelpStr; // The descriptive text message for -help
> + const char *ValueStr; // String describing what the value of this option is
> + OptionCategory *Category;// The Category this option belongs to
^ Space here?
> diff --git a/lib/Support/CommandLine.cpp b/lib/Support/CommandLine.cpp
> index 560d7eb..a8c7fc6 100644
> --- a/lib/Support/CommandLine.cpp
> +++ b/lib/Support/CommandLine.cpp
> @@ -33,6 +33,7 @@
> #include "llvm/Support/system_error.h"
> #include <cerrno>
> #include <cstdlib>
> +#include <map>
> using namespace llvm;
> using namespace cl;
>
> @@ -106,6 +107,17 @@ void Option::addArgument() {
> MarkOptionsChanged();
> }
>
> +// This collects the different option categories that have been registered.
> +typedef SmallPtrSet<OptionCategory*,16> OptionCatSet;
> +static ManagedStatic<OptionCatSet> RegisteredOptionCategories;
> +
> +//Initialise the general option category
^ Space here? ^ '.' here?
> //===----------------------------------------------------------------------===//
> // Basic, shared command line option processing machinery.
> @@ -528,6 +540,8 @@ static void ExpandResponseFiles(unsigned argc, const char*const* argv,
> }
> }
>
> +static void unHideHelpListOption();
> +
> void cl::ParseCommandLineOptions(int argc, const char * const *argv,
> const char *Overview) {
> // Process all registered options.
> @@ -539,6 +553,9 @@ void cl::ParseCommandLineOptions(int argc, const char * const *argv,
> assert((!Opts.empty() || !PositionalOpts.empty()) &&
> "No options specified!");
>
> + //Unhide -help-list if necessary
^ Space? ^ Point?
> + unHideHelpListOption();
> +
> // Expand response files.
> std::vector<char*> newArgv;
> newArgv.push_back(strdup(argv[0]));
> @@ -1224,6 +1241,15 @@ namespace {
> class HelpPrinter {
> const bool ShowHidden;
>
> +protected:
> + typedef SmallVector<std::pair<const char *, Option*>,128> StrOptionPairVector;
> + // Print the options. Opts is assumed to be alphabetically sorted.
> + virtual void printOptions(StrOptionPairVector &Opts, size_t MaxArgLen)
> + {
^ '{' on the previous line.
> + for (size_t i = 0, e = Opts.size(); i != e; ++i)
> + Opts[i].second->printOptionInfo(MaxArgLen);
> + }
> +
> public:
> explicit HelpPrinter(bool showHidden) : ShowHidden(showHidden) {}
>
> @@ -1236,7 +1262,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 +1293,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.
Moving this into a separate function looks reasonable.
>
> // Print any extra help the user has declared.
> for (std::vector<const char *>::iterator I = MoreHelp->begin(),
> @@ -1280,21 +1305,153 @@ 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.
You may want to mention that this function uses an alphabetic
(lexicographic) order to sort the options.
> + 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.
> + // 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.
> + 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.
> + // Sort the different option groups alphabetically.
> + assert(SortedCategories.size() > 0 && "No option categories registered!");
> + std::sort(SortedCategories.begin(), SortedCategories.end(),
> + OptionCategoryCompare);
> +
> + // Create map to empty vectors.
> + for (std::vector<OptionCategory*>::const_iterator
> + I = SortedCategories.begin(),
> + E = SortedCategories.end();
> + I != E ; ++I)
> + CategorizedOptions[*I] = std::vector<Option*>();
> +
> + // Walk through pre-sorted options and assign into groups.
... assign them to their
option group.
> + // Because the options are already alphabetically sorted the
> + // options within groups will also be alphabetically sorted.
within each group
> + // Now do printing
> + for (std::vector<OptionCategory*>::const_iterator
> + Category = SortedCategories.begin(),
> + E = SortedCategories.end();
> + Category != E; ++Category) {
> + //print category information
^ Space, start with uppercase and add '.' at the end.
> + outs() << "\n";
> + outs() << (*Category)->getName() << ":\n";
> +
> + // Check if description is set
^ Missing '.'
> + if ((*Category)->getDescription() != 0)
> + outs() << (*Category)->getDescription() << "\n\n";
> + else
> + outs() << "\n";
> +
> + // Loop over the options in the category and print
> + for (std::vector<Option*>::const_iterator
> + Opt = CategorizedOptions[*Category].begin(),
> + E = CategorizedOptions[*Category].end();
> + Opt != E; ++Opt)
> + (*Opt)->printOptionInfo(MaxArgLen);
> +
> + }
> +
Unnecessary empty newline.
> + }
> +};
> +
> +// This wraps the Uncategorizing and Categorizing printers and decides
> +// at run time which should be invoked.
> +class HelpPrinterWrapper {
> +private:
> + HelpPrinter &UncategorizedPrinter;
> + CategorizedHelpPrinter &CategorizedPrinter;
> +
> +public:
> + explicit HelpPrinterWrapper(HelpPrinter &UncategorizedPrinter,
> + CategorizedHelpPrinter &CategorizedPrinter,
> + Option *ToUnhide=0) :
The argument 'ToUnhide' seems to be unused?
> + UncategorizedPrinter(UncategorizedPrinter),
> + CategorizedPrinter(CategorizedPrinter) { }
> +
> + void 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)
> + CategorizedPrinter=true; // Invoke printer
> + else
> + UncategorizedPrinter=true; // Invoke printer
> + }
> +
> +};
> +
> } // End anonymous namespace
>
> -// Define the two HelpPrinter instances that are used to print out help, or
> -// help-hidden...
> -//
> -static HelpPrinter NormalPrinter(false);
> -static HelpPrinter HiddenPrinter(true);
> +// Define the four HelpPrinter instances that are used to print out help, or
> +// help-hidden as an uncategorized list or in categories.
> +static HelpPrinter UncategorizedNormalPrinter(false);
> +static HelpPrinter UncategorizedHiddenPrinter(true);
> +static CategorizedHelpPrinter CategorizedNormalPrinter(false);
> +static CategorizedHelpPrinter CategorizedHiddenPrinter(true);
> +
> +;
> +
> +// Define HelpPrinter wrappers that will decide whether or not to invoke
> +// a categorizing help printer
> +static HelpPrinterWrapper WrappedNormalPrinter(UncategorizedNormalPrinter,
> + CategorizedNormalPrinter);
> +static HelpPrinterWrapper WrappedHiddenPrinter(UncategorizedHiddenPrinter,
> + CategorizedHiddenPrinter);
> +
> +// Define uncategorized help printers.
> +// -help-list is hidden by default because if Option categories are being used
> +// then -help behaves the same as -help-list.
> +static cl::opt<HelpPrinter, true, parser<bool> >
> +HLOp("help-list",
> + cl::desc("Display list of available options (-help-list-hidden for more)"),
> + cl::location(UncategorizedNormalPrinter), cl::Hidden, cl::ValueDisallowed);
>
> static cl::opt<HelpPrinter, true, parser<bool> >
> +HLHOp("help-list-hidden",
> + cl::desc("Display list of all available options"),
> + cl::location(UncategorizedHiddenPrinter), cl::Hidden, cl::ValueDisallowed);
> +
> +// Define uncategorized/categorized help printers.
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.
All the best,
Tobias
More information about the llvm-commits
mailing list