[clang] Migrate clang-rename to OptTable parsing #89167
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Using OptTable to parse will allow including this tool in llvm-driver. Because CommonOptionsParser is widely used and makes use of `cl::opt` flags, it needs to be refactored to handle both. The existing `CommonOptionsParser::create()` method should continue to work for downstream users. An additional overload allows a general function to be passed in, which can do arg parsing however it likes, as long as it returns the fields that CommonOptionsParser needs. Many other simple `clang-*` tools can be similarly migrated after this.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Jordan Rupprecht (rupprecht) ChangesUsing OptTable to parse will allow including this tool in llvm-driver. Because CommonOptionsParser is widely used and makes use of Many other simple Patch is 23.29 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/89167.diff 7 Files Affected:
diff --git a/clang/include/clang/Tooling/CommonOptionsParser.h b/clang/include/clang/Tooling/CommonOptionsParser.h
index 3c0480af377943..da1fd299a04836 100644
--- a/clang/include/clang/Tooling/CommonOptionsParser.h
+++ b/clang/include/clang/Tooling/CommonOptionsParser.h
@@ -86,6 +86,24 @@ class CommonOptionsParser {
llvm::cl::NumOccurrencesFlag OccurrencesFlag = llvm::cl::OneOrMore,
const char *Overview = nullptr);
+ struct Args {
+ std::string BuildPath;
+ std::vector<std::string> SourcePaths;
+ std::vector<std::string> ArgsAfter;
+ std::vector<std::string> ArgsBefore;
+ };
+
+ using ArgParserCallback =
+ std::function<llvm::Expected<Args>(int &argc, const char **argv)>;
+
+ /// A factory method that is similar to the above factory method, except
+ /// this does not force use of cl::opt argument parsing. The function passed
+ /// in is expected to handle argument parsing, and must return values needed
+ /// by CommonOptionsParser.
+ static llvm::Expected<CommonOptionsParser>
+ create(int &argc, const char **argv, ArgParserCallback ArgsCallback,
+ llvm::cl::NumOccurrencesFlag OccurrencesFlag = llvm::cl::OneOrMore);
+
/// Returns a reference to the loaded compilations database.
CompilationDatabase &getCompilations() {
return *Compilations;
@@ -105,10 +123,9 @@ class CommonOptionsParser {
private:
CommonOptionsParser() = default;
- llvm::Error init(int &argc, const char **argv,
- llvm::cl::OptionCategory &Category,
- llvm::cl::NumOccurrencesFlag OccurrencesFlag,
- const char *Overview);
+ llvm::Error
+ init(int &argc, const char **argv, ArgParserCallback ArgsCallback,
+ llvm::cl::NumOccurrencesFlag OccurrencesFlag = llvm::cl::OneOrMore);
std::unique_ptr<CompilationDatabase> Compilations;
std::vector<std::string> SourcePathList;
diff --git a/clang/include/clang/Tooling/CommonOptionsParser.td b/clang/include/clang/Tooling/CommonOptionsParser.td
new file mode 100644
index 00000000000000..0e79136a42bcb2
--- /dev/null
+++ b/clang/include/clang/Tooling/CommonOptionsParser.td
@@ -0,0 +1,14 @@
+include "llvm/Option/OptParser.td"
+
+multiclass Eq<string name, string help> {
+ def NAME#_EQ : Joined<["--", "-"], name#"=">, HelpText<help>;
+ def : Separate<["--", "-"], name>, Alias<!cast<Joined>(NAME#_EQ)>;
+}
+
+defm build_path : Eq<"p", "Build path.">;
+defm extra_arg
+ : Eq<"extra-arg",
+ "Additional argument to append to the compiler command line.">;
+defm extra_arg_before
+ : Eq<"extra-arg-before",
+ "Additional argument to prepend to the compiler command line.">;
diff --git a/clang/lib/Tooling/CommonOptionsParser.cpp b/clang/lib/Tooling/CommonOptionsParser.cpp
index 59ef47cc0166ef..341a8ab46c50e7 100644
--- a/clang/lib/Tooling/CommonOptionsParser.cpp
+++ b/clang/lib/Tooling/CommonOptionsParser.cpp
@@ -57,13 +57,12 @@ void ArgumentsAdjustingCompilations::appendArgumentsAdjuster(
Adjusters.push_back(std::move(Adjuster));
}
-std::vector<CompileCommand> ArgumentsAdjustingCompilations::getCompileCommands(
- StringRef FilePath) const {
+std::vector<CompileCommand>
+ArgumentsAdjustingCompilations::getCompileCommands(StringRef FilePath) const {
return adjustCommands(Compilations->getCompileCommands(FilePath));
}
-std::vector<std::string>
-ArgumentsAdjustingCompilations::getAllFiles() const {
+std::vector<std::string> ArgumentsAdjustingCompilations::getAllFiles() const {
return Compilations->getAllFiles();
}
@@ -80,58 +79,32 @@ std::vector<CompileCommand> ArgumentsAdjustingCompilations::adjustCommands(
return Commands;
}
-llvm::Error CommonOptionsParser::init(
- int &argc, const char **argv, cl::OptionCategory &Category,
- llvm::cl::NumOccurrencesFlag OccurrencesFlag, const char *Overview) {
-
- static cl::opt<std::string> BuildPath("p", cl::desc("Build path"),
- cl::Optional, cl::cat(Category),
- cl::sub(cl::SubCommand::getAll()));
-
- static cl::list<std::string> SourcePaths(
- cl::Positional, cl::desc("<source0> [... <sourceN>]"), OccurrencesFlag,
- cl::cat(Category), cl::sub(cl::SubCommand::getAll()));
-
- static cl::list<std::string> ArgsAfter(
- "extra-arg",
- cl::desc("Additional argument to append to the compiler command line"),
- cl::cat(Category), cl::sub(cl::SubCommand::getAll()));
-
- static cl::list<std::string> ArgsBefore(
- "extra-arg-before",
- cl::desc("Additional argument to prepend to the compiler command line"),
- cl::cat(Category), cl::sub(cl::SubCommand::getAll()));
-
- cl::ResetAllOptionOccurrences();
-
- cl::HideUnrelatedOptions(Category);
-
+llvm::Error
+CommonOptionsParser::init(int &argc, const char **argv,
+ ArgParserCallback ArgsCallback,
+ llvm::cl::NumOccurrencesFlag OccurrencesFlag) {
std::string ErrorMessage;
Compilations =
FixedCompilationDatabase::loadFromCommandLine(argc, argv, ErrorMessage);
if (!ErrorMessage.empty())
ErrorMessage.append("\n");
- llvm::raw_string_ostream OS(ErrorMessage);
- // Stop initializing if command-line option parsing failed.
- if (!cl::ParseCommandLineOptions(argc, argv, Overview, &OS)) {
- OS.flush();
- return llvm::make_error<llvm::StringError>(ErrorMessage,
- llvm::inconvertibleErrorCode());
- }
- cl::PrintOptionValues();
+ // Stop initializing if command-line option parsing failed.
+ auto Args = ArgsCallback(argc, argv);
+ if (!Args)
+ return Args.takeError();
- SourcePathList = SourcePaths;
+ SourcePathList = Args->SourcePaths;
if ((OccurrencesFlag == cl::ZeroOrMore || OccurrencesFlag == cl::Optional) &&
SourcePathList.empty())
return llvm::Error::success();
if (!Compilations) {
- if (!BuildPath.empty()) {
- Compilations =
- CompilationDatabase::autoDetectFromDirectory(BuildPath, ErrorMessage);
+ if (!Args->BuildPath.empty()) {
+ Compilations = CompilationDatabase::autoDetectFromDirectory(
+ Args->BuildPath, ErrorMessage);
} else {
- Compilations = CompilationDatabase::autoDetectFromSource(SourcePaths[0],
- ErrorMessage);
+ Compilations = CompilationDatabase::autoDetectFromSource(
+ Args->SourcePaths[0], ErrorMessage);
}
if (!Compilations) {
llvm::errs() << "Error while trying to load a compilation database:\n"
@@ -141,24 +114,72 @@ llvm::Error CommonOptionsParser::init(
}
}
auto AdjustingCompilations =
- std::make_unique<ArgumentsAdjustingCompilations>(
- std::move(Compilations));
- Adjuster =
- getInsertArgumentAdjuster(ArgsBefore, ArgumentInsertPosition::BEGIN);
+ std::make_unique<ArgumentsAdjustingCompilations>(std::move(Compilations));
+ Adjuster = getInsertArgumentAdjuster(Args->ArgsBefore,
+ ArgumentInsertPosition::BEGIN);
Adjuster = combineAdjusters(
std::move(Adjuster),
- getInsertArgumentAdjuster(ArgsAfter, ArgumentInsertPosition::END));
+ getInsertArgumentAdjuster(Args->ArgsAfter, ArgumentInsertPosition::END));
AdjustingCompilations->appendArgumentsAdjuster(Adjuster);
Compilations = std::move(AdjustingCompilations);
return llvm::Error::success();
}
+CommonOptionsParser::ArgParserCallback
+makeClOptParserCallback(llvm::cl::OptionCategory &Category,
+ llvm::cl::NumOccurrencesFlag OccurrencesFlag,
+ const char *Overview) {
+ return [&Category, OccurrencesFlag, Overview](
+ int &argc,
+ const char **argv) -> llvm::Expected<CommonOptionsParser::Args> {
+ static cl::opt<std::string> BuildPath("p", cl::desc("Build path"),
+ cl::Optional, cl::cat(Category),
+ cl::sub(cl::SubCommand::getAll()));
+
+ static cl::list<std::string> SourcePaths(
+ cl::Positional, cl::desc("<source0> [... <sourceN>]"), OccurrencesFlag,
+ cl::cat(Category), cl::sub(cl::SubCommand::getAll()));
+
+ static cl::list<std::string> ArgsAfter(
+ "extra-arg",
+ cl::desc("Additional argument to append to the compiler command line"),
+ cl::cat(Category), cl::sub(cl::SubCommand::getAll()));
+
+ static cl::list<std::string> ArgsBefore(
+ "extra-arg-before",
+ cl::desc("Additional argument to prepend to the compiler command line"),
+ cl::cat(Category), cl::sub(cl::SubCommand::getAll()));
+
+ cl::ResetAllOptionOccurrences();
+
+ cl::HideUnrelatedOptions(Category);
+
+ std::string ErrorMessage;
+ llvm::raw_string_ostream OS(ErrorMessage);
+ if (!cl::ParseCommandLineOptions(argc, argv, Overview, &OS)) {
+ OS.flush();
+ return llvm::make_error<llvm::StringError>(
+ ErrorMessage, llvm::inconvertibleErrorCode());
+ }
+ return CommonOptionsParser::Args{BuildPath, SourcePaths, ArgsAfter,
+ ArgsBefore};
+ };
+}
+
llvm::Expected<CommonOptionsParser> CommonOptionsParser::create(
int &argc, const char **argv, llvm::cl::OptionCategory &Category,
llvm::cl::NumOccurrencesFlag OccurrencesFlag, const char *Overview) {
+ return create(argc, argv,
+ makeClOptParserCallback(Category, OccurrencesFlag, Overview),
+ OccurrencesFlag);
+}
+
+llvm::Expected<CommonOptionsParser>
+CommonOptionsParser::create(int &argc, const char **argv,
+ ArgParserCallback ArgsCallback,
+ llvm::cl::NumOccurrencesFlag OccurrencesFlag) {
CommonOptionsParser Parser;
- llvm::Error Err =
- Parser.init(argc, argv, Category, OccurrencesFlag, Overview);
+ llvm::Error Err = Parser.init(argc, argv, ArgsCallback, OccurrencesFlag);
if (Err)
return std::move(Err);
return std::move(Parser);
@@ -167,7 +188,9 @@ llvm::Expected<CommonOptionsParser> CommonOptionsParser::create(
CommonOptionsParser::CommonOptionsParser(
int &argc, const char **argv, cl::OptionCategory &Category,
llvm::cl::NumOccurrencesFlag OccurrencesFlag, const char *Overview) {
- llvm::Error Err = init(argc, argv, Category, OccurrencesFlag, Overview);
+ llvm::Error Err = init(
+ argc, argv, makeClOptParserCallback(Category, OccurrencesFlag, Overview),
+ OccurrencesFlag);
if (Err) {
llvm::report_fatal_error(
Twine("CommonOptionsParser: failed to parse command-line arguments. ") +
diff --git a/clang/tools/clang-rename/CMakeLists.txt b/clang/tools/clang-rename/CMakeLists.txt
index f4c4e520520d9e..b67cc4ee0c84f8 100644
--- a/clang/tools/clang-rename/CMakeLists.txt
+++ b/clang/tools/clang-rename/CMakeLists.txt
@@ -3,8 +3,15 @@ set(LLVM_LINK_COMPONENTS
Support
)
+set(LLVM_TARGET_DEFINITIONS Opts.td)
+tablegen(LLVM Opts.inc -gen-opt-parser-defs)
+add_public_tablegen_target(ClangRenameOptsTableGen)
+
add_clang_tool(clang-rename
ClangRename.cpp
+
+ DEPENDS
+ ClangRenameOptsTableGen
)
clang_target_link_libraries(clang-rename
diff --git a/clang/tools/clang-rename/ClangRename.cpp b/clang/tools/clang-rename/ClangRename.cpp
index f2ac0c4360e0dc..99b530dd6d2c2d 100644
--- a/clang/tools/clang-rename/ClangRename.cpp
+++ b/clang/tools/clang-rename/ClangRename.cpp
@@ -28,13 +28,20 @@
#include "clang/Tooling/ReplacementsYaml.h"
#include "clang/Tooling/Tooling.h"
#include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/Option/Arg.h"
+#include "llvm/Option/ArgList.h"
+#include "llvm/Option/OptTable.h"
+#include "llvm/Option/Option.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/WithColor.h"
#include "llvm/Support/YAMLTraits.h"
#include "llvm/Support/raw_ostream.h"
#include <string>
#include <system_error>
+#include "Opts.inc"
+
using namespace llvm;
using namespace clang;
@@ -63,45 +70,123 @@ template <> struct MappingTraits<RenameAllInfo> {
} // end namespace yaml
} // end namespace llvm
+namespace {
+enum ID {
+ OPT_INVALID = 0, // This is not an option ID.
+#define OPTION(...) LLVM_MAKE_OPT_ID(__VA_ARGS__),
+#include "Opts.inc"
+#undef OPTION
+};
+
+#define PREFIX(NAME, VALUE) \
+ static constexpr llvm::StringLiteral NAME##_init[] = VALUE; \
+ static constexpr llvm::ArrayRef<llvm::StringLiteral> NAME( \
+ NAME##_init, std::size(NAME##_init) - 1);
+#include "Opts.inc"
+#undef PREFIX
+
+using namespace llvm::opt;
+static constexpr opt::OptTable::Info InfoTable[] = {
+#define OPTION(...) LLVM_CONSTRUCT_OPT_INFO(__VA_ARGS__),
+#include "Opts.inc"
+#undef OPTION
+};
+
+class ClangRenameOptTable : public opt::GenericOptTable {
+public:
+ ClangRenameOptTable() : GenericOptTable(InfoTable) {}
+};
+} // end anonymous namespace
+
static cl::OptionCategory ClangRenameOptions("clang-rename common options");
-static cl::list<unsigned> SymbolOffsets(
- "offset",
- cl::desc("Locates the symbol by offset as opposed to <line>:<column>."),
- cl::cat(ClangRenameOptions));
-static cl::opt<bool> Inplace("i", cl::desc("Overwrite edited <file>s."),
- cl::cat(ClangRenameOptions));
-static cl::list<std::string>
- QualifiedNames("qualified-name",
- cl::desc("The fully qualified name of the symbol."),
- cl::cat(ClangRenameOptions));
-
-static cl::list<std::string>
- NewNames("new-name", cl::desc("The new name to change the symbol to."),
- cl::cat(ClangRenameOptions));
-static cl::opt<bool> PrintName(
- "pn",
- cl::desc("Print the found symbol's name prior to renaming to stderr."),
- cl::cat(ClangRenameOptions));
-static cl::opt<bool> PrintLocations(
- "pl", cl::desc("Print the locations affected by renaming to stderr."),
- cl::cat(ClangRenameOptions));
-static cl::opt<std::string>
- ExportFixes("export-fixes",
- cl::desc("YAML file to store suggested fixes in."),
- cl::value_desc("filename"), cl::cat(ClangRenameOptions));
-static cl::opt<std::string>
- Input("input", cl::desc("YAML file to load oldname-newname pairs from."),
- cl::Optional, cl::cat(ClangRenameOptions));
-static cl::opt<bool> Force("force",
- cl::desc("Ignore nonexistent qualified names."),
- cl::cat(ClangRenameOptions));
+static std::vector<unsigned> SymbolOffsets;
+static bool Inplace;
+static std::vector<std::string> QualifiedNames;
+static std::vector<std::string> NewNames;
+static bool PrintName;
+static bool PrintLocations;
+static std::string ExportFixes;
+static std::string Input;
+static bool Force;
+
+static tooling::CommonOptionsParser::Args ParseArgs(int argc,
+ const char **argv) {
+ ClangRenameOptTable Tbl;
+ llvm::StringRef ToolName = argv[0];
+ llvm::BumpPtrAllocator A;
+ llvm::StringSaver Saver{A};
+ llvm::opt::InputArgList Args = Tbl.parseArgs(
+ argc, const_cast<char **>(argv), OPT_UNKNOWN, Saver, [&](StringRef Msg) {
+ WithColor::error() << Msg << "\n";
+ std::exit(1);
+ });
+
+ if (Args.hasArg(OPT_help)) {
+ Tbl.printHelp(llvm::outs(), "clang-rename [options]", "clang-rename");
+ std::exit(0);
+ }
+ if (Args.hasArg(OPT_version)) {
+ llvm::outs() << ToolName << '\n';
+ llvm::cl::PrintVersionMessage();
+ std::exit(0);
+ }
+
+ for (const llvm::opt::Arg *A : Args.filtered(OPT_offset_EQ)) {
+ StringRef S{A->getValue()};
+ unsigned Value;
+ if (!llvm::to_integer(S, Value, 0)) {
+ WithColor::error() << ToolName << ": for the --offset option: '" << S
+ << "' value invalid for uint argument!\n";
+ std::exit(1);
+ }
+ SymbolOffsets.emplace_back(Value);
+ }
+
+ Inplace = Args.hasArg(OPT_inplace);
+
+ for (const llvm::opt::Arg *A : Args.filtered(OPT_qualified_name_EQ))
+ QualifiedNames.emplace_back(A->getValue());
+ for (const llvm::opt::Arg *A : Args.filtered(OPT_new_name_EQ))
+ NewNames.emplace_back(A->getValue());
+
+ PrintName = Args.hasArg(OPT_print_name);
+ PrintLocations = Args.hasArg(OPT_print_locations);
+
+ if (const llvm::opt::Arg *A = Args.getLastArg(OPT_export_fixes_EQ))
+ ExportFixes = A->getValue();
+ if (const llvm::opt::Arg *A = Args.getLastArg(OPT_input_EQ))
+ Input = A->getValue();
+
+ Force = Args.hasArg(OPT_force);
+
+ tooling::CommonOptionsParser::Args args;
+ if (const llvm::opt::Arg *A = Args.getLastArg(OPT_build_path_EQ))
+ args.BuildPath = A->getValue();
+ for (const llvm::opt::Arg *A : Args.filtered(OPT_extra_arg_EQ))
+ args.ArgsAfter.emplace_back(A->getValue());
+ for (const llvm::opt::Arg *A : Args.filtered(OPT_extra_arg_before_EQ))
+ args.ArgsBefore.emplace_back(A->getValue());
+ for (const llvm::opt::Arg *A : Args.filtered(OPT_INPUT))
+ args.SourcePaths.emplace_back(A->getValue());
+ if (args.SourcePaths.empty()) {
+ WithColor::error() << ToolName
+ << ": must set at least one source path (-p).\n";
+ std::exit(1);
+ }
+ return args;
+}
int main(int argc, const char **argv) {
- auto ExpectedParser =
- tooling::CommonOptionsParser::create(argc, argv, ClangRenameOptions);
+ auto callback = [&](int &argc, const char **argv)
+ -> llvm::Expected<tooling::CommonOptionsParser::Args> {
+ return ParseArgs(argc, argv);
+ };
+
+ auto ExpectedParser = tooling::CommonOptionsParser::create(
+ argc, const_cast<const char **>(argv), callback);
if (!ExpectedParser) {
- llvm::errs() << ExpectedParser.takeError();
+ WithColor::error() << ExpectedParser.takeError();
return 1;
}
tooling::CommonOptionsParser &OP = ExpectedParser.get();
@@ -111,8 +196,8 @@ int main(int argc, const char **argv) {
ErrorOr<std::unique_ptr<MemoryBuffer>> Buffer =
llvm::MemoryBuffer::getFile(Input);
if (!Buffer) {
- errs() << "clang-rename: failed to read " << Input << ": "
- << Buffer.getError().message() << "\n";
+ WithColor::error() << "clang-rename: failed to read " << Input << ": "
+ << Buffer.getError().message() << "\n";
return 1;
}
@@ -130,13 +215,14 @@ int main(int argc, const char **argv) {
// Check the arguments for correctness.
if (NewNames.empty()) {
- errs() << "clang-rename: -new-name must be specified.\n\n";
+ WithColor::error() << "clang-rename: -new-name must be specified.\n\n";
return 1;
}
if (SymbolOffsets.empty() == QualifiedNames.empty()) {
- errs() << "clang-rename: -offset and -qualified-name can't be present at "
- "the same time.\n";
+ WithColor::error()
+ << "clang-rename: -offset and -qualified-name can't be present at "
+ "the same time.\n";
return 1;
}
@@ -148,16 +234,19 @@ int main(int argc, const char **argv) {
for (const auto &NewName : NewNames) {
auto NewNameTokKind = Table.get(NewName).getTokenID();
if (!tok::isAnyIdentifier(NewNameTokKind)) {
- errs() << "ERROR: new name is not a valid identifier in C++17.\n\n";
+ WithColor::error()
+ << "ERROR: new name is not a valid identifier in C++17.\n\n";
return 1;
}
}
if (SymbolOffsets.size() + QualifiedNames.size() != NewNames.size()) {
- errs() << "clang-rename: number of symbol offsets(" << SymbolOffsets.size()
- << ") + number of qualified names (" << QualifiedNames.size()
- << ") must be equal to number of new names(" << NewNames.size()
- << ").\n\n";
+ WithColor::error() << "clang-rename: number of symbol offsets("
+ << SymbolOffsets.size()
+ << ") + number of qualified names ("
+ << QualifiedNames.size()
+ << ") must be equal to number of new names("
+ << NewNames.size() << ").\n\n";
cl::PrintHelpMessage();
return 1;
}
@@ -196,7 +285,8 @@ int main(int argc, const char **argv) {
std::error_code EC;
llvm::raw_fd_ostream OS(ExportFixes, EC, llvm::sys::fs::OF_None);
if (EC) {
- llvm::errs() << "Error opening output file: " << EC.message() << '\n';
+ WithColor::error() << "E...
[truncated]
|
sam-mccall
reviewed
Apr 18, 2024
There was a problem hiding this comment.
I'm not sold on the use of OptTable here, and think we should try some alternatives. I don't want to be a burden, so I'm happy to try this out if you like.
If it's just this tool then it's not that important, but I assume it's not.
There's possible scopes here of busyboxing: llvm (done) -> clang/tools -> clang-tools-extra -> out-of-tree tools. My guess is clang/tools is all in scope, out-of-tree tools are clearly not, unsure about clang-tools-extra.
busyboxing per se means mostly that tools have to give up on defining conflicting symbols, on having different things in the registries between tools, on global constructors, and have to live with main being a separate possibly-generated file. That all seems fine.
OptTable specifically has issues:
- it can't really be used downstream, so there's a big seam somewhere where similar tools do different things, and the in-tree tools aren't good models to follow.
- it apparently requires a lot of new generic boilerplate in every tool, and it's opaque xmacro stuff
- each flag is handled three times (in tablegen, in opt iteration, and as a data structure to parse into)
- it adds a third flags mechanism to clang tools, making debugging/escaping even harder (today cl::opt covers CommonOptionParser's flags, we also have the clang flags consumed via FixedCompilationDatabase)
- often requires learning a new language (as tablegen isn't widely used in the clang-tools world)
- extra build complexity: cmake/bazel/gn configs gain extra complexity that must be synced and debugged
- risk of subtle changes in flag parsing when migrating tools that currently use cl::opt
OptTable doesn't seem to be required:
- the entrypoint is the unopinionated
tool_main(argc,argv**), that doesn't require all tools to share a flag parsing strategy - cl::opt can parse from argc/argv fine
- global registration is the main problem, but can be avoided with relatively minor source changes (if flags are in main.cpp, but OptTable also requires this)
- global registration can be effectively detected with the use of
-Werror=global-constructorsas MLIR does. This will require some cleanup as I'm sure we have other violations. (The cleanup does provide some general value though.)
concretely I think we could replace existing usage:
cl::opt<string> Foo("foo");
int main() {
ParseCommandLineOptions();
print(Foo);
}
with this:
struct Flags {
cl::opt<string> Foo("foo");
};
const Flags& flags() {
static Flags flags;
return flags;
}
int main() {
flags();
ParseCommandLineOptions();
print(flags().Foo);
}
There's some additional boilerplate, but it's O(1).
I think the both the migration and the after state would be preferable for the tools currently using cl::opt.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Using OptTable to parse will allow including this tool in llvm-driver.
Because CommonOptionsParser is widely used and makes use of
cl::optflags, it needs to be refactored to handle both. The existingCommonOptionsParser::create()method should continue to work for downstream users. An additional overload allows a general function to be passed in, which can do arg parsing however it likes, as long as it returns the fields that CommonOptionsParser needs.Many other simple
clang-*tools can be similarly migrated after this.