[PATCH] Part 1 of Many: Refactoring llvm command line parsing
Chris Bieneman
beanz at apple.com
Tue Jan 20 17:23:20 PST 2015
> On Jan 20, 2015, at 4:40 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>
>
>> On 2015 Jan 20, at 16:09, Chris Bieneman <beanz at apple.com> wrote:
>>
>> Hi chandlerc,
>>
>> The primary goal of this patch is to remove the need for MarkOptionsChanged(). That goal is accomplished by having addOption and removeOption properly sort the options.
>>
>> This patch puts the new add and remove functionality on a CommandLineParser class that is a placeholder. Some of the functionality in this class will need to be merged into the OptionRegistry, and other bits can hopefully be in a better abstraction.
>>
>> Other changes:
>> * Minor changes to MachinePassRegistry.cpp, LegacyPassNameParser.cpp, and Pass.cpp to support changes to Parser.Initailize()
>> * Exposed a new API for tools to hide unrelated options (I have patches for clang to adopt)
>> *Fixed an issue with CommandLineTest caused by the new registration model.
>
> Can any of these be split out (particularly the new API seems like
> a candidate)?
There are a few things I can carve out into separate patches. As it is now, it all kind of organically grew out of my attempt to eliminate MarkOptionsChanged().
Basically the three bullets at the end could be carved out separately. They are changes that were needed in order for me to be able to eliminate MarkOptionsChanged, but they aren’t intrinsically bound to the change.
The new API came about because with this change the previous implementation of cl::getRegisteredOptions didn’t make sense. In changing that API I noticed that the two places it was in use by clang were both doing the same thing. It made more sense to me to add a new API (cl::HideUnrelatedOptions) rather than keeping clang tied to how cl::opt was implemented.
>
>>
>> http://reviews.llvm.org/D7076
>>
>> Files:
>> include/llvm/CodeGen/MachinePassRegistry.h
>> include/llvm/IR/LegacyPassNameParser.h
>> include/llvm/Support/CommandLine.h
>> lib/IR/Pass.cpp
>> lib/Support/CommandLine.cpp
>> unittests/Support/CommandLineTest.cpp
>>
>> EMAIL PREFERENCES
>> http://reviews.llvm.org/settings/panel/emailpreferences/
>> <D7076.18465.patch>_______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>> Index: include/llvm/CodeGen/MachinePassRegistry.h
>> ===================================================================
>> --- include/llvm/CodeGen/MachinePassRegistry.h
>> +++ include/llvm/CodeGen/MachinePassRegistry.h
>> @@ -122,11 +122,11 @@
>> class RegisterPassParser : public MachinePassRegistryListener,
>> public cl::parser<typename RegistryClass::FunctionPassCtor> {
>> public:
>> - RegisterPassParser() {}
>> + RegisterPassParser(cl::Option* O) : cl::parser<typename RegistryClass::FunctionPassCtor>(O) {}
>
> Should be `cl::Option *O`; also 80-column. (There are a bunch of other
> formatting nitpicks. Clang format?)
I did clang-format on CommandLine.h/.cpp. Forgot to run it on my changes elsewhere.
>
>> ~RegisterPassParser() { RegistryClass::setListener(nullptr); }
>>
>> - void initialize(cl::Option &O) {
>> - cl::parser<typename RegistryClass::FunctionPassCtor>::initialize(O);
>> + void initialize() {
>> + cl::parser<typename RegistryClass::FunctionPassCtor>::initialize();
>>
>> // Add existing passes to option.
>> for (RegistryClass *Node = RegistryClass::getList();
>> Index: include/llvm/IR/LegacyPassNameParser.h
>> ===================================================================
>> --- include/llvm/IR/LegacyPassNameParser.h
>> +++ include/llvm/IR/LegacyPassNameParser.h
>> @@ -41,14 +41,12 @@
>> //
>> class PassNameParser : public PassRegistrationListener,
>> public cl::parser<const PassInfo*> {
>> - cl::Option *Opt;
>> public:
>> - PassNameParser();
>> + PassNameParser(cl::Option* O);
>> virtual ~PassNameParser();
>>
>> - void initialize(cl::Option &O) {
>> - Opt = &O;
>> - cl::parser<const PassInfo*>::initialize(O);
>> + void initialize() {
>> + cl::parser<const PassInfo*>::initialize();
>>
>> // Add all of the passes to the map that got initialized before 'this' did.
>> enumeratePasses();
>> @@ -69,7 +67,7 @@
>> // Implement the PassRegistrationListener callbacks used to populate our map
>> //
>> void passRegistered(const PassInfo *P) override {
>> - if (ignorablePass(P) || !Opt) return;
>> + if (ignorablePass(P) || !Owner) return;
>> if (findOption(P->getPassArgument()) != getNumOptions()) {
>> errs() << "Two passes with the same argument (-"
>> << P->getPassArgument() << ") attempted to be registered!\n";
>> Index: include/llvm/Support/CommandLine.h
>> ===================================================================
>> --- include/llvm/Support/CommandLine.h
>> +++ include/llvm/Support/CommandLine.h
>> @@ -72,9 +72,6 @@
>> // (Currently not perfect, but best-effort.)
>> void PrintOptionValues();
>>
>> -// MarkOptionsChanged - Internal helper function.
>> -void MarkOptionsChanged();
>> -
>> //===----------------------------------------------------------------------===//
>> // Flags permitted to be passed to command line arguments
>> //
>> @@ -529,8 +526,11 @@
>> const char *Name;
>> const char *HelpStr;
>> };
>> + Option *Owner;
>
> Is this ever null? Can it change after construction?
Good call. It can’t be null or change. I can make it a reference.
>
> If not, I wonder if a reference would be more clear. (Same question for
> much of the API, where `Option *`s are being passed around. I feel like
> an `Option &` would be more clear in a lot of those cases.)
Most of the existing API is pointer based, and I don’t think it makes sense to rework existing API. The only real new API that takes a pointer is AddLiteralOption, which could be done either way. I had it as a pointer to match the Parser::addLiteralOption API which calls it.
>
>>
>> public:
>> + generic_parser_base(Option *O) : Owner(O){};
>
> Stray semi-colon.
I’ll clean these all up.
>
>> +
>> virtual ~generic_parser_base() {} // Base class should have virtual-dtor
>>
>> // getNumOptions - Virtual function implemented by generic subclass to
>> @@ -569,18 +569,13 @@
>> printGenericOptionDiff(O, V, Default, GlobalWidth);
>> }
>>
>> - void initialize(Option &O) {
>> - // All of the modifiers for the option have been processed by now, so the
>> - // argstr field should be stable, copy it down now.
>> - //
>> - hasArgStr = O.hasArgStr();
>> - }
>> + void initialize() {}
>>
>> void getExtraOptionNames(SmallVectorImpl<const char *> &OptionNames) {
>> // If there has been no argstr specified, that means that we need to add an
>> // argument for every possible option. This ensures that our options are
>> // vectored to us.
>> - if (!hasArgStr)
>> + if (!Owner->hasArgStr())
>> for (unsigned i = 0, e = getNumOptions(); i != e; ++i)
>> OptionNames.push_back(getOption(i));
>> }
>> @@ -597,7 +592,7 @@
>> //
>> // If this is the case, we cannot allow a value.
>> //
>> - if (hasArgStr)
>> + if (Owner->hasArgStr())
>> return ValueRequired;
>> else
>> return ValueDisallowed;
>> @@ -607,9 +602,6 @@
>> // argument string. If the option is not found, getNumOptions() is returned.
>> //
>> unsigned findOption(const char *Name);
>> -
>> -protected:
>> - bool hasArgStr;
>> };
>>
>> // Default parser implementation - This implementation depends on having a
>> @@ -629,6 +621,7 @@
>> SmallVector<OptionInfo, 8> Values;
>>
>> public:
>> + parser(Option *O) : generic_parser_base(O){};
>> typedef DataType parser_data_type;
>>
>> // Implement virtual functions needed by generic_parser_base
>> @@ -646,7 +639,7 @@
>> // parse - Return true on error.
>> bool parse(Option &O, StringRef ArgName, StringRef Arg, DataType &V) {
>> StringRef ArgVal;
>> - if (hasArgStr)
>> + if (Owner->hasArgStr())
>> ArgVal = Arg;
>> else
>> ArgVal = ArgName;
>> @@ -667,7 +660,7 @@
>> assert(findOption(Name) == Values.size() && "Option already exists!");
>> OptionInfo X(Name, static_cast<DataType>(V), HelpStr);
>> Values.push_back(X);
>> - MarkOptionsChanged();
>> + AddLiteralOption(Owner, Name);
>> }
>>
>> /// removeLiteralOption - Remove the specified option.
>> @@ -684,15 +677,17 @@
>> //
>> class basic_parser_impl { // non-template implementation of basic_parser<t>
>> public:
>> + basic_parser_impl(Option *O) : Owner(O){};
>
> Stray semi-colon. These will kill -Werror builds, I think.
>
>> +
>> virtual ~basic_parser_impl() {}
>>
>> enum ValueExpected getValueExpectedFlagDefault() const {
>> return ValueRequired;
>> }
>>
>> void getExtraOptionNames(SmallVectorImpl<const char *> &) {}
>>
>> - void initialize(Option &) {}
>> + void initialize() {}
>>
>> // Return the width of the option tag for printing...
>> size_t getOptionWidth(const Option &O) const;
>> @@ -715,13 +710,16 @@
>> protected:
>> // A helper for basic_parser::printOptionDiff.
>> void printOptionName(const Option &O, size_t GlobalWidth) const;
>> +
>> + Option *Owner;
>> };
>>
>> // basic_parser - The real basic parser is just a template wrapper that provides
>> // a typedef for the provided data type.
>> //
>> template <class DataType> class basic_parser : public basic_parser_impl {
>> public:
>> + basic_parser(Option *O) : basic_parser_impl(O){};
>
> Another semi-colon.
>
>> typedef DataType parser_data_type;
>> typedef OptionValue<DataType> OptVal;
>> };
>> @@ -733,10 +731,12 @@
>> const char *ArgStr;
>>
>> public:
>> + parser(Option *O) : basic_parser(O){};
>
> Another semi-colon.
>
>> +
>> // parse - Return true on error.
>> bool parse(Option &O, StringRef ArgName, StringRef Arg, bool &Val);
>>
>> - template <class Opt> void initialize(Opt &O) { ArgStr = O.ArgStr; }
>> + void initialize() { ArgStr = Owner->ArgStr; }
>>
>> enum ValueExpected getValueExpectedFlagDefault() const {
>> return ValueOptional;
>> @@ -758,6 +758,8 @@
>> // parser<boolOrDefault>
>> template <> class parser<boolOrDefault> : public basic_parser<boolOrDefault> {
>> public:
>> + parser(Option *O) : basic_parser(O){};
>> +
>> // parse - Return true on error.
>> bool parse(Option &O, StringRef ArgName, StringRef Arg, boolOrDefault &Val);
>>
>> @@ -782,6 +784,8 @@
>> //
>> template <> class parser<int> : public basic_parser<int> {
>> public:
>> + parser(Option *O) : basic_parser(O){};
>> +
>> // parse - Return true on error.
>> bool parse(Option &O, StringRef ArgName, StringRef Arg, int &Val);
>>
>> @@ -802,6 +806,8 @@
>> //
>> template <> class parser<unsigned> : public basic_parser<unsigned> {
>> public:
>> + parser(Option *O) : basic_parser(O){};
>
> Another semi-colon.
>
>> +
>> // parse - Return true on error.
>> bool parse(Option &O, StringRef ArgName, StringRef Arg, unsigned &Val);
>>
>> @@ -823,6 +829,8 @@
>> template <>
>> class parser<unsigned long long> : public basic_parser<unsigned long long> {
>> public:
>> + parser(Option *O) : basic_parser(O){};
>
> Another semi-colon. (I'll stop pointing these out.)
>
>> +
>> // parse - Return true on error.
>> bool parse(Option &O, StringRef ArgName, StringRef Arg,
>> unsigned long long &Val);
>> @@ -844,6 +852,8 @@
>> //
>> template <> class parser<double> : public basic_parser<double> {
>> public:
>> + parser(Option *O) : basic_parser(O){};
>> +
>> // parse - Return true on error.
>> bool parse(Option &O, StringRef ArgName, StringRef Arg, double &Val);
>>
>> @@ -864,6 +874,8 @@
>> //
>> template <> class parser<float> : public basic_parser<float> {
>> public:
>> + parser(Option *O) : basic_parser(O){};
>> +
>> // parse - Return true on error.
>> bool parse(Option &O, StringRef ArgName, StringRef Arg, float &Val);
>>
>> @@ -884,6 +896,8 @@
>> //
>> template <> class parser<std::string> : public basic_parser<std::string> {
>> public:
>> + parser(Option *O) : basic_parser(O){};
>> +
>> // parse - Return true on error.
>> bool parse(Option &, StringRef, StringRef Arg, std::string &Value) {
>> Value = Arg.str();
>> @@ -907,6 +921,8 @@
>> //
>> template <> class parser<char> : public basic_parser<char> {
>> public:
>> + parser(Option *O) : basic_parser(O){};
>> +
>> // parse - Return true on error.
>> bool parse(Option &, StringRef, StringRef Arg, char &Value) {
>> Value = Arg[0];
>> @@ -1166,7 +1182,7 @@
>>
>> void done() {
>> addArgument();
>> - Parser.initialize(*this);
>> + Parser.initialize();
>> }
>>
>> public:
>> @@ -1183,33 +1199,33 @@
>> // One option...
>> template <class M0t>
>> explicit opt(const M0t &M0)
>> - : Option(Optional, NotHidden) {
>> + : Option(Optional, NotHidden), Parser(this) {
>> apply(M0, this);
>> done();
>> }
>>
>> // Two options...
>> template <class M0t, class M1t>
>> opt(const M0t &M0, const M1t &M1)
>> - : Option(Optional, NotHidden) {
>> + : Option(Optional, NotHidden), Parser(this) {
>> apply(M0, this);
>> apply(M1, this);
>> done();
>> }
>>
>> // Three options...
>> template <class M0t, class M1t, class M2t>
>> opt(const M0t &M0, const M1t &M1, const M2t &M2)
>> - : Option(Optional, NotHidden) {
>> + : Option(Optional, NotHidden), Parser(this) {
>> apply(M0, this);
>> apply(M1, this);
>> apply(M2, this);
>> done();
>> }
>> // Four options...
>> template <class M0t, class M1t, class M2t, class M3t>
>> opt(const M0t &M0, const M1t &M1, const M2t &M2, const M3t &M3)
>> - : Option(Optional, NotHidden) {
>> + : Option(Optional, NotHidden), Parser(this) {
>> apply(M0, this);
>> apply(M1, this);
>> apply(M2, this);
>> @@ -1219,7 +1235,7 @@
>> // Five options...
>> template <class M0t, class M1t, class M2t, class M3t, class M4t>
>> opt(const M0t &M0, const M1t &M1, const M2t &M2, const M3t &M3, const M4t &M4)
>> - : Option(Optional, NotHidden) {
>> + : Option(Optional, NotHidden), Parser(this) {
>> apply(M0, this);
>> apply(M1, this);
>> apply(M2, this);
>> @@ -1231,7 +1247,7 @@
>> template <class M0t, class M1t, class M2t, class M3t, class M4t, class M5t>
>> opt(const M0t &M0, const M1t &M1, const M2t &M2, const M3t &M3, const M4t &M4,
>> const M5t &M5)
>> - : Option(Optional, NotHidden) {
>> + : Option(Optional, NotHidden), Parser(this) {
>> apply(M0, this);
>> apply(M1, this);
>> apply(M2, this);
>> @@ -1245,7 +1261,7 @@
>> class M6t>
>> opt(const M0t &M0, const M1t &M1, const M2t &M2, const M3t &M3, const M4t &M4,
>> const M5t &M5, const M6t &M6)
>> - : Option(Optional, NotHidden) {
>> + : Option(Optional, NotHidden), Parser(this) {
>> apply(M0, this);
>> apply(M1, this);
>> apply(M2, this);
>> @@ -1260,7 +1276,7 @@
>> class M6t, class M7t>
>> opt(const M0t &M0, const M1t &M1, const M2t &M2, const M3t &M3, const M4t &M4,
>> const M5t &M5, const M6t &M6, const M7t &M7)
>> - : Option(Optional, NotHidden) {
>> + : Option(Optional, NotHidden), Parser(this) {
>> apply(M0, this);
>> apply(M1, this);
>> apply(M2, this);
>> @@ -1361,7 +1377,7 @@
>>
>> void done() {
>> addArgument();
>> - Parser.initialize(*this);
>> + Parser.initialize();
>> }
>>
>> public:
>> @@ -1377,31 +1393,31 @@
>> // One option...
>> template <class M0t>
>> explicit list(const M0t &M0)
>> - : Option(ZeroOrMore, NotHidden) {
>> + : Option(ZeroOrMore, NotHidden), Parser(this) {
>> apply(M0, this);
>> done();
>> }
>> // Two options...
>> template <class M0t, class M1t>
>> list(const M0t &M0, const M1t &M1)
>> - : Option(ZeroOrMore, NotHidden) {
>> + : Option(ZeroOrMore, NotHidden), Parser(this) {
>> apply(M0, this);
>> apply(M1, this);
>> done();
>> }
>> // Three options...
>> template <class M0t, class M1t, class M2t>
>> list(const M0t &M0, const M1t &M1, const M2t &M2)
>> - : Option(ZeroOrMore, NotHidden) {
>> + : Option(ZeroOrMore, NotHidden), Parser(this) {
>> apply(M0, this);
>> apply(M1, this);
>> apply(M2, this);
>> done();
>> }
>> // Four options...
>> template <class M0t, class M1t, class M2t, class M3t>
>> list(const M0t &M0, const M1t &M1, const M2t &M2, const M3t &M3)
>> - : Option(ZeroOrMore, NotHidden) {
>> + : Option(ZeroOrMore, NotHidden), Parser(this) {
>> apply(M0, this);
>> apply(M1, this);
>> apply(M2, this);
>> @@ -1412,7 +1428,7 @@
>> template <class M0t, class M1t, class M2t, class M3t, class M4t>
>> list(const M0t &M0, const M1t &M1, const M2t &M2, const M3t &M3,
>> const M4t &M4)
>> - : Option(ZeroOrMore, NotHidden) {
>> + : Option(ZeroOrMore, NotHidden), Parser(this) {
>> apply(M0, this);
>> apply(M1, this);
>> apply(M2, this);
>> @@ -1424,7 +1440,7 @@
>> template <class M0t, class M1t, class M2t, class M3t, class M4t, class M5t>
>> list(const M0t &M0, const M1t &M1, const M2t &M2, const M3t &M3,
>> const M4t &M4, const M5t &M5)
>> - : Option(ZeroOrMore, NotHidden) {
>> + : Option(ZeroOrMore, NotHidden), Parser(this) {
>> apply(M0, this);
>> apply(M1, this);
>> apply(M2, this);
>> @@ -1438,7 +1454,7 @@
>> class M6t>
>> list(const M0t &M0, const M1t &M1, const M2t &M2, const M3t &M3,
>> const M4t &M4, const M5t &M5, const M6t &M6)
>> - : Option(ZeroOrMore, NotHidden) {
>> + : Option(ZeroOrMore, NotHidden), Parser(this) {
>> apply(M0, this);
>> apply(M1, this);
>> apply(M2, this);
>> @@ -1453,7 +1469,7 @@
>> class M6t, class M7t>
>> list(const M0t &M0, const M1t &M1, const M2t &M2, const M3t &M3,
>> const M4t &M4, const M5t &M5, const M6t &M6, const M7t &M7)
>> - : Option(ZeroOrMore, NotHidden) {
>> + : Option(ZeroOrMore, NotHidden), Parser(this) {
>> apply(M0, this);
>> apply(M1, this);
>> apply(M2, this);
>> @@ -1581,7 +1597,7 @@
>>
>> void done() {
>> addArgument();
>> - Parser.initialize(*this);
>> + Parser.initialize();
>> }
>>
>> public:
>> @@ -1595,31 +1611,31 @@
>> // One option...
>> template <class M0t>
>> explicit bits(const M0t &M0)
>> - : Option(ZeroOrMore, NotHidden) {
>> + : Option(ZeroOrMore, NotHidden), Parser(this) {
>> apply(M0, this);
>> done();
>> }
>> // Two options...
>> template <class M0t, class M1t>
>> bits(const M0t &M0, const M1t &M1)
>> - : Option(ZeroOrMore, NotHidden) {
>> + : Option(ZeroOrMore, NotHidden), Parser(this) {
>> apply(M0, this);
>> apply(M1, this);
>> done();
>> }
>> // Three options...
>> template <class M0t, class M1t, class M2t>
>> bits(const M0t &M0, const M1t &M1, const M2t &M2)
>> - : Option(ZeroOrMore, NotHidden) {
>> + : Option(ZeroOrMore, NotHidden), Parser(this) {
>> apply(M0, this);
>> apply(M1, this);
>> apply(M2, this);
>> done();
>> }
>> // Four options...
>> template <class M0t, class M1t, class M2t, class M3t>
>> bits(const M0t &M0, const M1t &M1, const M2t &M2, const M3t &M3)
>> - : Option(ZeroOrMore, NotHidden) {
>> + : Option(ZeroOrMore, NotHidden), Parser(this) {
>> apply(M0, this);
>> apply(M1, this);
>> apply(M2, this);
>> @@ -1630,7 +1646,7 @@
>> template <class M0t, class M1t, class M2t, class M3t, class M4t>
>> bits(const M0t &M0, const M1t &M1, const M2t &M2, const M3t &M3,
>> const M4t &M4)
>> - : Option(ZeroOrMore, NotHidden) {
>> + : Option(ZeroOrMore, NotHidden), Parser(this) {
>> apply(M0, this);
>> apply(M1, this);
>> apply(M2, this);
>> @@ -1642,7 +1658,7 @@
>> template <class M0t, class M1t, class M2t, class M3t, class M4t, class M5t>
>> bits(const M0t &M0, const M1t &M1, const M2t &M2, const M3t &M3,
>> const M4t &M4, const M5t &M5)
>> - : Option(ZeroOrMore, NotHidden) {
>> + : Option(ZeroOrMore, NotHidden), Parser(this) {
>> apply(M0, this);
>> apply(M1, this);
>> apply(M2, this);
>> @@ -1656,7 +1672,7 @@
>> class M6t>
>> bits(const M0t &M0, const M1t &M1, const M2t &M2, const M3t &M3,
>> const M4t &M4, const M5t &M5, const M6t &M6)
>> - : Option(ZeroOrMore, NotHidden) {
>> + : Option(ZeroOrMore, NotHidden), Parser(this) {
>> apply(M0, this);
>> apply(M1, this);
>> apply(M2, this);
>> @@ -1671,7 +1687,7 @@
>> class M6t, class M7t>
>> bits(const M0t &M0, const M1t &M1, const M2t &M2, const M3t &M3,
>> const M4t &M4, const M5t &M5, const M6t &M6, const M7t &M7)
>> - : Option(ZeroOrMore, NotHidden) {
>> + : Option(ZeroOrMore, NotHidden), Parser(this) {
>> apply(M0, this);
>> apply(M1, this);
>> apply(M2, this);
>> @@ -1818,7 +1834,11 @@
>> /// This interface is useful for modifying options in libraries that are out of
>> /// the control of the client. The options should be modified before calling
>> /// llvm::cl::ParseCommandLineOptions().
>> -void getRegisteredOptions(StringMap<Option *> &Map);
>> +///
>> +/// Hopefully this API can be depricated soon. Any situation where options need
>> +/// to be modified by tools or libraries should be handled by sane APIs rather
>> +/// than just handing around a global list.
>> +StringMap<Option *> &getRegisteredOptions();
>>
>> //===----------------------------------------------------------------------===//
>> // Standalone command line processing utilities.
>> @@ -1889,6 +1909,24 @@
>> SmallVectorImpl<const char *> &Argv,
>> bool MarkEOLs = false);
>>
>> +/// \brief Adds a new option for parsing and provides the option it refers to.
>> +///
>> +/// \param O pointer to the option
>> +/// \param Name the string name for the option to handle during parsing
>> +///
>> +/// Literal options are used by some parsers to register special option values.
>> +/// This is how the PassNameParser registers pass names for opt.
>> +void AddLiteralOption(Option *O, const char *Name);
>> +
>> +/// \brief Mark all options not part of this category as cl::ReallyHidden.
>> +///
>> +/// \param Category the category of options to keep displaying
>> +///
>> +/// Some tools (like clang-format) like to be able to hide all options that are
>> +/// not specific to the tool. This function allows a tool to specify a single
>> +/// option category to display in the -help output.
>> +void HideUnrelatedOptions(cl::OptionCategory &Category);
>> +
>> } // End namespace cl
>>
>> } // End namespace llvm
>> Index: lib/IR/Pass.cpp
>> ===================================================================
>> --- lib/IR/Pass.cpp
>> +++ lib/IR/Pass.cpp
>> @@ -223,8 +223,8 @@
>> PassRegistry::getPassRegistry()->enumerateWith(this);
>> }
>>
>> -PassNameParser::PassNameParser()
>> - : Opt(nullptr) {
>> +PassNameParser::PassNameParser(cl::Option* O)
>> + : cl::parser<const PassInfo*>(O) {
>> PassRegistry::getPassRegistry()->addRegistrationListener(this);
>> }
>>
>> Index: lib/Support/CommandLine.cpp
>> ===================================================================
>> --- lib/Support/CommandLine.cpp
>> +++ lib/Support/CommandLine.cpp
>> @@ -83,48 +83,117 @@
>>
>> //===----------------------------------------------------------------------===//
>>
>> -// Globals for name and overview of program. Program name is not a string to
>> -// avoid static ctor/dtor issues.
>> -static char ProgramName[80] = "<premain>";
>> -static const char *ProgramOverview = nullptr;
>> +class CommandLineParser {
>> +public:
>> + // Globals for name and overview of program. Program name is not a string to
>> + // avoid static ctor/dtor issues.
>> + char ProgramName[80] = "<premain>";
>> + const char *ProgramOverview = nullptr;
>>
>> -// This collects additional help to be printed.
>> -static ManagedStatic<std::vector<const char *>> MoreHelp;
>> + // This collects additional help to be printed.
>> + std::vector<const char *> MoreHelp;
>>
>> -extrahelp::extrahelp(const char *Help) : morehelp(Help) {
>> - MoreHelp->push_back(Help);
>> -}
>> + SmallVector<Option *, 4> PositionalOpts;
>> + SmallVector<Option *, 4> SinkOpts;
>> + StringMap<Option *> OptionsMap;
>>
>> -static bool OptionListChanged = false;
>> + Option *ConsumeAfterOpt = nullptr; // The ConsumeAfter option if it exists.
>>
>> -// MarkOptionsChanged - Internal helper function.
>> -void cl::MarkOptionsChanged() { OptionListChanged = true; }
>> + void ParseCommandLineOptions(int argc, const char *const *argv,
>> + const char *Overview);
>>
>> -/// RegisteredOptionList - This is the list of the command line options that
>> -/// have statically constructed themselves.
>> -static Option *RegisteredOptionList = nullptr;
>> + void addLiteralOption(Option *Opt, const char *Name) {
>> + if (!Opt->hasArgStr()) {
>> + if (!OptionsMap.insert(std::make_pair(Name, Opt)).second) {
>> + errs() << ProgramName << ": CommandLine Error: Option '" << Name
>> + << "' registered more than once!\n";
>> + report_fatal_error("inconsistency in registered CommandLine options");
>> + }
>> + }
>> + }
>>
>> -void Option::addArgument() {
>> - assert(!NextRegistered && "argument multiply registered!");
>> + void addOption(Option *O) {
>> + bool HadErrors = false;
>> + if (O->ArgStr[0]) {
>> + // Add argument to the argument map!
>> + if (!OptionsMap.insert(std::make_pair(O->ArgStr, O)).second) {
>> + errs() << ProgramName << ": CommandLine Error: Option '" << O->ArgStr
>> + << "' registered more than once!\n";
>> + HadErrors = true;
>> + }
>> + }
>>
>> - NextRegistered = RegisteredOptionList;
>> - RegisteredOptionList = this;
>> - MarkOptionsChanged();
>> -}
>> + // Remember information about positional options.
>> + if (O->getFormattingFlag() == cl::Positional)
>> + PositionalOpts.push_back(O);
>> + else if (O->getMiscFlags() & cl::Sink) // Remember sink options
>> + SinkOpts.push_back(O);
>> + else if (O->getNumOccurrencesFlag() == cl::ConsumeAfter) {
>> + if (ConsumeAfterOpt) {
>> + O->error("Cannot specify more than one option with cl::ConsumeAfter!");
>> + HadErrors = true;
>> + }
>> + ConsumeAfterOpt = O;
>> + }
>>
>> -void Option::removeArgument() {
>> - if (RegisteredOptionList == this) {
>> - RegisteredOptionList = NextRegistered;
>> - MarkOptionsChanged();
>> - return;
>> + // Fail hard if there were errors. These are strictly unrecoverable and
>> + // indicate serious issues such as conflicting option names or an
>> + // incorrectly
>> + // linked LLVM distribution.
>> + if (HadErrors)
>> + report_fatal_error("inconsistency in registered CommandLine options");
>> + }
>> +
>> + void removeOption(Option *O) {
>> + SmallVector<const char *, 16> OptionNames;
>> + O->getExtraOptionNames(OptionNames);
>> + if (O->ArgStr[0])
>> + OptionNames.push_back(O->ArgStr);
>> + for (auto Name : OptionNames)
>> + OptionsMap.erase(StringRef(Name));
>> +
>> + if (O->getFormattingFlag() == cl::Positional)
>> + for (auto Opt = PositionalOpts.begin(); Opt != PositionalOpts.end();
>> + ++Opt) {
>> + if (*Opt == O) {
>> + PositionalOpts.erase(Opt);
>> + break;
>> + }
>> + }
>> + else if (O->getMiscFlags() & cl::Sink)
>> + for (auto Opt = SinkOpts.begin(); Opt != SinkOpts.end(); ++Opt) {
>> + if (*Opt == O) {
>> + SinkOpts.erase(Opt);
>> + break;
>> + }
>> + }
>> + else if (O == ConsumeAfterOpt)
>> + ConsumeAfterOpt = nullptr;
>> }
>> - Option *O = RegisteredOptionList;
>> - for (; O->NextRegistered != this; O = O->NextRegistered)
>> - ;
>> - O->NextRegistered = NextRegistered;
>> - MarkOptionsChanged();
>> +
>> + bool hasOptions() {
>> + return (!OptionsMap.empty() || !PositionalOpts.empty() ||
>> + nullptr != ConsumeAfterOpt);
>> + }
>> +};
>> +
>> +static ManagedStatic<CommandLineParser> GlobalParser;
>> +
>> +void cl::AddLiteralOption(Option *O, const char *Name) {
>> + GlobalParser->addLiteralOption(O, Name);
>> +}
>> +
>> +extrahelp::extrahelp(const char *Help) : morehelp(Help) {
>> + GlobalParser->MoreHelp.push_back(Help);
>> }
>>
>> +void Option::addArgument() {
>> + assert(!NextRegistered && "argument multiply registered!");
>> + GlobalParser->addOption(this);
>> +}
>> +
>> +void Option::removeArgument() { GlobalParser->removeOption(this); }
>> +
>> // This collects the different option categories that have been registered.
>> typedef SmallPtrSet<OptionCategory *, 16> OptionCatSet;
>> static ManagedStatic<OptionCatSet> RegisteredOptionCategories;
>> @@ -147,60 +216,6 @@
>> // Basic, shared command line option processing machinery.
>> //
>>
>> -/// GetOptionInfo - Scan the list of registered options, turning them into data
>> -/// structures that are easier to handle.
>> -static void GetOptionInfo(SmallVectorImpl<Option *> &PositionalOpts,
>> - SmallVectorImpl<Option *> &SinkOpts,
>> - StringMap<Option *> &OptionsMap) {
>> - bool HadErrors = false;
>> - SmallVector<const char *, 16> OptionNames;
>> - Option *CAOpt = nullptr; // The ConsumeAfter option if it exists.
>> - for (Option *O = RegisteredOptionList; O; O = O->getNextRegisteredOption()) {
>> - // If this option wants to handle multiple option names, get the full set.
>> - // This handles enum options like "-O1 -O2" etc.
>> - O->getExtraOptionNames(OptionNames);
>> - if (O->ArgStr[0])
>> - OptionNames.push_back(O->ArgStr);
>> -
>> - // Handle named options.
>> - for (size_t i = 0, e = OptionNames.size(); i != e; ++i) {
>> - // Add argument to the argument map!
>> - if (!OptionsMap.insert(std::make_pair(OptionNames[i], O)).second) {
>> - errs() << ProgramName << ": CommandLine Error: Option '"
>> - << OptionNames[i] << "' registered more than once!\n";
>> - HadErrors = true;
>> - }
>> - }
>> -
>> - OptionNames.clear();
>> -
>> - // Remember information about positional options.
>> - if (O->getFormattingFlag() == cl::Positional)
>> - PositionalOpts.push_back(O);
>> - else if (O->getMiscFlags() & cl::Sink) // Remember sink options
>> - SinkOpts.push_back(O);
>> - else if (O->getNumOccurrencesFlag() == cl::ConsumeAfter) {
>> - if (CAOpt) {
>> - O->error("Cannot specify more than one option with cl::ConsumeAfter!");
>> - HadErrors = true;
>> - }
>> - CAOpt = O;
>> - }
>> - }
>> -
>> - if (CAOpt)
>> - PositionalOpts.push_back(CAOpt);
>> -
>> - // Make sure that they are in order of registration not backwards.
>> - std::reverse(PositionalOpts.begin(), PositionalOpts.end());
>> -
>> - // Fail hard if there were errors. These are strictly unrecoverable and
>> - // indicate serious issues such as conflicting option names or an incorrectly
>> - // linked LLVM distribution.
>> - if (HadErrors)
>> - report_fatal_error("inconsistency in registered CommandLine options");
>> -}
>> -
>> /// LookupOption - Lookup the option specified by the specified option on the
>> /// command line. If there is a value specified (after an equal sign) return
>> /// that as well. This assumes that leading dashes have already been stripped.
>> @@ -777,13 +792,13 @@
>>
>> void cl::ParseCommandLineOptions(int argc, const char *const *argv,
>> const char *Overview) {
>> - // Process all registered options.
>> - SmallVector<Option *, 4> PositionalOpts;
>> - SmallVector<Option *, 4> SinkOpts;
>> - StringMap<Option *> Opts;
>> - GetOptionInfo(PositionalOpts, SinkOpts, Opts);
>> + GlobalParser->ParseCommandLineOptions(argc, argv, Overview);
>> +}
>>
>> - assert((!Opts.empty() || !PositionalOpts.empty()) && "No options specified!");
>> +void CommandLineParser::ParseCommandLineOptions(int argc,
>> + const char *const *argv,
>> + const char *Overview) {
>> + assert(hasOptions() && "No options specified!");
>>
>> // Expand response files.
>> SmallVector<const char *, 20> newArgv;
>> @@ -809,25 +824,22 @@
>> // Determine whether or not there are an unlimited number of positionals
>> bool HasUnlimitedPositionals = false;
>>
>> - Option *ConsumeAfterOpt = nullptr;
>> + if (ConsumeAfterOpt) {
>> + assert(PositionalOpts.size() > 0 &&
>> + "Cannot specify cl::ConsumeAfter without a positional argument!");
>> + }
>> if (!PositionalOpts.empty()) {
>> - if (PositionalOpts[0]->getNumOccurrencesFlag() == cl::ConsumeAfter) {
>> - assert(PositionalOpts.size() > 1 &&
>> - "Cannot specify cl::ConsumeAfter without a positional argument!");
>> - ConsumeAfterOpt = PositionalOpts[0];
>> - }
>>
>> // Calculate how many positional values are _required_.
>> bool UnboundedFound = false;
>> - for (size_t i = ConsumeAfterOpt ? 1 : 0, e = PositionalOpts.size(); i != e;
>> - ++i) {
>> + for (size_t i = 0, e = PositionalOpts.size(); i != e; ++i) {
>> Option *Opt = PositionalOpts[i];
>> if (RequiresValue(Opt))
>> ++NumPositionalRequired;
>> else if (ConsumeAfterOpt) {
>> // ConsumeAfter cannot be combined with "optional" positional options
>> // unless there is only one positional argument...
>> - if (PositionalOpts.size() > 2)
>> + if (PositionalOpts.size() > 1)
>> ErrorParsing |= Opt->error(
>> "error - this positional option will never be matched, "
>> "because it does not Require a value, and a "
>> @@ -841,6 +853,9 @@
>> "another positional argument will match an "
>> "unbounded number of values, and this option"
>> " does not require a value!");
>> + errs() << ProgramName << ": CommandLine Error: Option '" << Opt->ArgStr
>> + << "' is all messed up!\n";
>> + errs() << PositionalOpts.size();
>> }
>> UnboundedFound |= EatsUnboundedNumberOfValues(Opt);
>> }
>> @@ -866,17 +881,6 @@
>> StringRef Value;
>> StringRef ArgName = "";
>>
>> - // If the option list changed, this means that some command line
>> - // option has just been registered or deregistered. This can occur in
>> - // response to things like -load, etc. If this happens, rescan the options.
>> - if (OptionListChanged) {
>> - PositionalOpts.clear();
>> - SinkOpts.clear();
>> - Opts.clear();
>> - GetOptionInfo(PositionalOpts, SinkOpts, Opts);
>> - OptionListChanged = false;
>> - }
>> -
>> // Check to see if this is a positional argument. This argument is
>> // considered to be positional if it doesn't start with '-', if it is "-"
>> // itself, or if we have seen "--" already.
>> @@ -917,7 +921,7 @@
>> while (!ArgName.empty() && ArgName[0] == '-')
>> ArgName = ArgName.substr(1);
>>
>> - Handler = LookupOption(ArgName, Value, Opts);
>> + Handler = LookupOption(ArgName, Value, OptionsMap);
>> if (!Handler || Handler->getFormattingFlag() != cl::Positional) {
>> ProvidePositionalOption(ActivePositionalArg, argv[i], i);
>> continue; // We are done!
>> @@ -929,18 +933,18 @@
>> while (!ArgName.empty() && ArgName[0] == '-')
>> ArgName = ArgName.substr(1);
>>
>> - Handler = LookupOption(ArgName, Value, Opts);
>> + Handler = LookupOption(ArgName, Value, OptionsMap);
>>
>> // Check to see if this "option" is really a prefixed or grouped argument.
>> if (!Handler)
>> - Handler =
>> - HandlePrefixedOrGroupedOption(ArgName, Value, ErrorParsing, Opts);
>> + Handler = HandlePrefixedOrGroupedOption(ArgName, Value, ErrorParsing,
>> + OptionsMap);
>>
>> // Otherwise, look for the closest available option to report to the user
>> // in the upcoming error.
>> if (!Handler && SinkOpts.empty())
>> NearestHandler =
>> - LookupNearestOption(ArgName, Opts, NearestHandlerString);
>> + LookupNearestOption(ArgName, OptionsMap, NearestHandlerString);
>> }
>>
>> if (!Handler) {
>> @@ -1037,8 +1041,8 @@
>> // positional option and keep the rest for the consume after. The above
>> // loop would have assigned no values to positional options in this case.
>> //
>> - if (PositionalOpts.size() == 2 && ValNo == 0 && !PositionalVals.empty()) {
>> - ErrorParsing |= ProvidePositionalOption(PositionalOpts[1],
>> + if (PositionalOpts.size() == 1 && ValNo == 0 && !PositionalVals.empty()) {
>> + ErrorParsing |= ProvidePositionalOption(PositionalOpts[0],
>> PositionalVals[ValNo].first,
>> PositionalVals[ValNo].second);
>> ValNo++;
>> @@ -1053,7 +1057,7 @@
>> }
>>
>> // Loop over args and make sure all required args are specified!
>> - for (const auto &Opt : Opts) {
>> + for (const auto &Opt : OptionsMap) {
>> switch (Opt.second->getNumOccurrencesFlag()) {
>> case Required:
>> case OneOrMore:
>> @@ -1076,9 +1080,7 @@
>>
>> // Free all of the memory allocated to the map. Command line options may only
>> // be processed once!
>> - Opts.clear();
>> - PositionalOpts.clear();
>> - MoreHelp->clear();
>> + MoreHelp.clear();
>>
>> // If we had an error processing our arguments, don't let the program execute
>> if (ErrorParsing)
>> @@ -1095,7 +1097,7 @@
>> if (ArgName.empty())
>> errs() << HelpStr; // Be nice for positional arguments
>> else
>> - errs() << ProgramName << ": for the -" << ArgName;
>> + errs() << GlobalParser->ProgramName << ": for the -" << ArgName;
>>
>> errs() << " option: " << Message << "\n";
>> return true;
>> @@ -1483,35 +1485,23 @@
>> if (Value == false)
>> return;
>>
>> - // Get all the options.
>> - SmallVector<Option *, 4> PositionalOpts;
>> - SmallVector<Option *, 4> SinkOpts;
>> - StringMap<Option *> OptMap;
>> - GetOptionInfo(PositionalOpts, SinkOpts, OptMap);
>> -
>> StrOptionPairVector Opts;
>> - sortOpts(OptMap, Opts, ShowHidden);
>> -
>> - if (ProgramOverview)
>> - outs() << "OVERVIEW: " << ProgramOverview << "\n";
>> + sortOpts(GlobalParser->OptionsMap, Opts, ShowHidden);
>>
>> - outs() << "USAGE: " << ProgramName << " [options]";
>> + if (GlobalParser->ProgramOverview)
>> + outs() << "OVERVIEW: " << GlobalParser->ProgramOverview << "\n";
>>
>> - // Print out the positional options.
>> - Option *CAOpt = nullptr; // The cl::ConsumeAfter option, if it exists...
>> - if (!PositionalOpts.empty() &&
>> - PositionalOpts[0]->getNumOccurrencesFlag() == ConsumeAfter)
>> - CAOpt = PositionalOpts[0];
>> + outs() << "USAGE: " << GlobalParser->ProgramName << " [options]";
>>
>> - for (size_t i = CAOpt != nullptr, e = PositionalOpts.size(); i != e; ++i) {
>> - if (PositionalOpts[i]->ArgStr[0])
>> - outs() << " --" << PositionalOpts[i]->ArgStr;
>> - outs() << " " << PositionalOpts[i]->HelpStr;
>> + for (size_t i = 0, e = GlobalParser->PositionalOpts.size(); i != e; ++i) {
>> + if (GlobalParser->PositionalOpts[i]->ArgStr[0])
>> + outs() << " --" << GlobalParser->PositionalOpts[i]->ArgStr;
>> + outs() << " " << GlobalParser->PositionalOpts[i]->HelpStr;
>> }
>>
>> // Print the consume after option info if it exists...
>> - if (CAOpt)
>> - outs() << " " << CAOpt->HelpStr;
>> + if (GlobalParser->ConsumeAfterOpt)
>> + outs() << " " << GlobalParser->ConsumeAfterOpt->HelpStr;
>>
>> outs() << "\n\n";
>>
>> @@ -1524,11 +1514,11 @@
>> printOptions(Opts, MaxArgLen);
>>
>> // Print any extra help the user has declared.
>> - for (std::vector<const char *>::iterator I = MoreHelp->begin(),
>> - E = MoreHelp->end();
>> + for (std::vector<const char *>::iterator I = GlobalParser->MoreHelp.begin(),
>> + E = GlobalParser->MoreHelp.end();
>> I != E; ++I)
>> outs() << *I;
>> - MoreHelp->clear();
>> + GlobalParser->MoreHelp.clear();
>>
>> // Halt the program since help information was printed
>> exit(0);
>> @@ -1709,14 +1699,8 @@
>> if (!PrintOptions && !PrintAllOptions)
>> return;
>>
>> - // Get all the options.
>> - SmallVector<Option *, 4> PositionalOpts;
>> - SmallVector<Option *, 4> SinkOpts;
>> - StringMap<Option *> OptMap;
>> - GetOptionInfo(PositionalOpts, SinkOpts, OptMap);
>> -
>> SmallVector<std::pair<const char *, Option *>, 128> Opts;
>> - sortOpts(OptMap, Opts, /*ShowHidden*/ true);
>> + sortOpts(GlobalParser->OptionsMap, Opts, /*ShowHidden*/ true);
>>
>> // Compute the maximum argument length...
>> size_t MaxArgLen = 0;
>> @@ -1823,13 +1807,18 @@
>> ExtraVersionPrinters->push_back(func);
>> }
>>
>> -void cl::getRegisteredOptions(StringMap<Option *> &Map) {
>> - // Get all the options.
>> - SmallVector<Option *, 4> PositionalOpts; // NOT USED
>> - SmallVector<Option *, 4> SinkOpts; // NOT USED
>> - assert(Map.size() == 0 && "StringMap must be empty");
>> - GetOptionInfo(PositionalOpts, SinkOpts, Map);
>> - return;
>> +StringMap<Option *> &cl::getRegisteredOptions() {
>> + return GlobalParser->OptionsMap;
>> +}
>> +
>> +void cl::HideUnrelatedOptions(cl::OptionCategory &Category) {
>> + for (StringMap<cl::Option *>::iterator I = GlobalParser->OptionsMap.begin(),
>> + E = GlobalParser->OptionsMap.end();
>> + I != E; ++I) {
>> + if (I->second->Category != &Category && I->first() != "help" &&
>> + I->first() != "version")
>> + I->second->setHiddenFlag(cl::ReallyHidden);
>> + }
>> }
>>
>> void LLVMParseCommandLineOptions(int argc, const char *const *argv,
>> Index: unittests/Support/CommandLineTest.cpp
>> ===================================================================
>> --- unittests/Support/CommandLineTest.cpp
>> +++ unittests/Support/CommandLineTest.cpp
>> @@ -70,14 +70,14 @@
>>
>>
>> cl::OptionCategory TestCategory("Test Options", "Description");
>> -cl::opt<int> TestOption("test-option", cl::desc("old description"));
>> TEST(CommandLineTest, ModifyExisitingOption) {
>> + cl::opt<int> TestOption("test-option", cl::desc("old description"));
>> +
>
> Why was this change necessary?
The command line parsing code doesn’t allow you to register multiple options with the same argument string. Before my changes this validation was only done when you called certain APIs like cl::ParseCommandLine. With my change that validation is done at option registration, which is part of the option constructor.
There is another option inside the scope of a test that has the argument string “test-option”, so I moved this option into the test scope so that it doesn’t conflict. It will be registered when the test begins executing and un-registered once the stack variable leaves scope.
>
>> const char Description[] = "New description";
>> const char ArgString[] = "new-test-option";
>> const char ValueString[] = "Integer";
>>
>> - StringMap<cl::Option*> Map;
>> - cl::getRegisteredOptions(Map);
>> + StringMap<cl::Option*> &Map = cl::getRegisteredOptions();
>
> You might as well update the spacing to `cl::Option*` since you've
> touched the line anyway.
>
>>
>> ASSERT_TRUE(Map.count("test-option") == 1) <<
>> "Could not find option in map.";
>>
>
More information about the llvm-commits
mailing list