[PATCH] Part 1 of Many: Refactoring llvm command line parsing
Duncan P. N. Exon Smith
dexonsmith at apple.com
Tue Jan 20 16:40:12 PST 2015
> 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)?
>
> 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?)
> ~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?
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.)
>
> public:
> + generic_parser_base(Option *O) : Owner(O){};
Stray semi-colon.
> +
> 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?
> 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