This is an archive of the discontinued LLVM Phabricator instance.

Refactoring llvm command line parsing and option registration.
ClosedPublic

Authored by beanz on Jan 22 2015, 2:29 PM.

Details

Summary

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.

This patch also removes the RegisteredOptionList global, and the need for cl::Option objects to be linked list nodes.

The changes in CommandLineTest.cpp are required because these changes shift when we validate that options are not duplicated. Before this change duplicate options were only found during certain cl API calls (like cl::ParseCommandLine). With this change duplicate options are found during option construction.

Diff Detail

Repository
rL LLVM

Event Timeline

beanz updated this revision to Diff 18639.Jan 22 2015, 2:29 PM
beanz retitled this revision from to Refactoring llvm command line parsing and option registration..
beanz updated this object.
beanz edited the test plan for this revision. (Show Details)
beanz added reviewers: dexonsmith, chandlerc.
beanz added a subscriber: Unknown Object (MLST).
majnemer added inline comments.
lib/Support/CommandLine.cpp
1495 ↗(On Diff #18639)

Range-based for might be more concise.

1516–1519 ↗(On Diff #18639)

Range-based for might be more concise.

beanz updated this revision to Diff 18644.Jan 22 2015, 4:46 PM

Converted two for loops to range-based for loops based on majnemer's feedback.

beanz updated this revision to Diff 18854.Jan 27 2015, 2:42 PM

Rebasing patches so they apply cleanly.

pete accepted this revision.Jan 27 2015, 4:59 PM
pete added a reviewer: pete.
pete added a subscriber: pete.

LGTM.

This revision is now accepted and ready to land.Jan 27 2015, 4:59 PM
This revision was automatically updated to reflect the committed changes.
Ilod added a subscriber: Ilod.Jan 28 2015, 1:53 PM

Hello,

This breaks the build on Visual 2013, with:
C:\Users\Ilod\pdb\llvm\lib\Support\CommandLine.cpp(90): error C2536: 'CommandLineParser::CommandLineParser::ProgramName' : cannot specify explicit initializer for arrays

You can't have an initializer for array or static array in the class definition in Visual 2013, you must do it in the constructor.

beanz added a comment.Jan 28 2015, 2:00 PM

I’ll put together a patch for this shortly.

-Chris

beanz added a comment.Jan 28 2015, 2:27 PM

Committed a fix for Visual Studio r227385.

-Chris

In D7132#114838, @beanz wrote:

Committed a fix for Visual Studio r227385.

-Chris

Hi Chris,

This doesn't seem to have fixed the failure:

(ClCompile target) -> 
  F:\work\src\llvm\lib\Support\CommandLine.cpp(91): error C2864: 'CommandLineParser::ProgramOverview' : only static const integral data members can be initialized within a class  ...
  F:\work\src\llvm\lib\Support\CommandLine.cpp(100): error C2864: 'CommandLineParser::ConsumeAfterOpt' : only static const integral data members can be initialized within a class ...

Moving the initializer to the (default) constructor does it:

diff --git a/lib/Support/CommandLine.cpp b/lib/Support/CommandLine.cpp
index 2ca9caa..a3d7d06 100644
--- a/lib/Support/CommandLine.cpp
+++ b/lib/Support/CommandLine.cpp
@@ -88,7 +88,7 @@ public:
   // Globals for name and overview of program.  Program name is not a string to
   // avoid static ctor/dtor issues.
   std::string ProgramName;
-  const char *ProgramOverview = nullptr;
+  const char *ProgramOverview;
 
   // This collects additional help to be printed.
   std::vector<const char *> MoreHelp;
@@ -97,7 +97,9 @@ public:
   SmallVector<Option *, 4> SinkOpts;
   StringMap<Option *> OptionsMap;
 
-  Option *ConsumeAfterOpt = nullptr; // The ConsumeAfter option if it exists.
+  Option *ConsumeAfterOpt; // The ConsumeAfter option if it exists.
+
+  CommandLineParser():ProgramOverview(nullptr), ConsumeAfterOpt(nullptr){}
 
   void ParseCommandLineOptions(int argc, const char *const *argv,
                                const char *Overview);
  • Asiri