[clang] [llvm] [clang] Migrate clang-rename to OptTable parsing (PR #89167)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall commented:

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-constructors` as 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 Foo("foo");

int main() {
  ParseCommandLineOptions();
  print(Foo);
}
```

with this:
```
struct Flags {
  cl::opt 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.

https://github.com/llvm/llvm-project/pull/89167
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang] Migrate clang-rename to OptTable parsing (PR #89167)

[via cfe-commits]

llvmbot wrote:




@llvm/pr-subscribers-clang

Author: Jordan Rupprecht (rupprecht)


Changes

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.

---

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:

- (modified) clang/include/clang/Tooling/CommonOptionsParser.h (+21-4) 
- (added) clang/include/clang/Tooling/CommonOptionsParser.td (+14) 
- (modified) clang/lib/Tooling/CommonOptionsParser.cpp (+75-52) 
- (modified) clang/tools/clang-rename/CMakeLists.txt (+7) 
- (modified) clang/tools/clang-rename/ClangRename.cpp (+136-46) 
- (added) clang/tools/clang-rename/Opts.td (+32) 
- (modified) utils/bazel/llvm-project-overlay/clang/BUILD.bazel (+19-2) 


``diff
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 SourcePaths;
+std::vector ArgsAfter;
+std::vector ArgsBefore;
+  };
+
+  using ArgParserCallback =
+  std::function(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
+  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 Compilations;
   std::vector SourcePathList;
diff --git a/clang/include/clang/Tooling/CommonOptionsParser.td 
b/clang/include/clang/Tooling/CommonOptionsParser.td
new file mode 100644
index 00..0e79136a42bcb2
--- /dev/null
+++ b/clang/include/clang/Tooling/CommonOptionsParser.td
@@ -0,0 +1,14 @@
+include "llvm/Option/OptParser.td"
+
+multiclass Eq {
+  def NAME#_EQ : Joined<["--", "-"], name#"=">, HelpText;
+  def : Separate<["--", "-"], name>, Alias(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 ArgumentsAdjustingCompilations::getCompileCommands(
-StringRef FilePath) const {
+std::vector
+ArgumentsAdjustingCompilations::getCompileCommands(StringRef FilePath) const {
   return adjustCommands(Compilations->getCompileCommands(FilePath));
 }
 
-std::vector
-ArgumentsAdjustingCompilations::getAllFiles() const {
+std::vector ArgumentsAdjustingCompilations::getAllFiles() const {
   return Compilations->getAllFiles();
 }
 
@@ -80,58 +79,32 @@ std::vector 
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 BuildPath("p", cl::desc("Build path"),
-cl::Optional, cl::cat(Category),
- 

[clang] [llvm] [clang] Migrate clang-rename to OptTable parsing (PR #89167)

[Jordan Rupprecht via cfe-commits]

https://github.com/rupprecht created 
https://github.com/llvm/llvm-project/pull/89167

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.

>From cdea18b434d09f47343716adaa9af9deee895770 Mon Sep 17 00:00:00 2001
From: Jordan Rupprecht 
Date: Thu, 18 Apr 2024 02:49:01 +
Subject: [PATCH] [clang] Migrate clang-rename to OptTable parsing

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.
---
 .../clang/Tooling/CommonOptionsParser.h   |  25 ++-
 .../clang/Tooling/CommonOptionsParser.td  |  14 ++
 clang/lib/Tooling/CommonOptionsParser.cpp | 127 +++-
 clang/tools/clang-rename/CMakeLists.txt   |   7 +
 clang/tools/clang-rename/ClangRename.cpp  | 182 +-
 clang/tools/clang-rename/Opts.td  |  32 +++
 .../llvm-project-overlay/clang/BUILD.bazel|  21 +-
 7 files changed, 304 insertions(+), 104 deletions(-)
 create mode 100644 clang/include/clang/Tooling/CommonOptionsParser.td
 create mode 100644 clang/tools/clang-rename/Opts.td

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 SourcePaths;
+std::vector ArgsAfter;
+std::vector ArgsBefore;
+  };
+
+  using ArgParserCallback =
+  std::function(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
+  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 Compilations;
   std::vector SourcePathList;
diff --git a/clang/include/clang/Tooling/CommonOptionsParser.td 
b/clang/include/clang/Tooling/CommonOptionsParser.td
new file mode 100644
index 00..0e79136a42bcb2
--- /dev/null
+++ b/clang/include/clang/Tooling/CommonOptionsParser.td
@@ -0,0 +1,14 @@
+include "llvm/Option/OptParser.td"
+
+multiclass Eq {
+  def NAME#_EQ : Joined<["--", "-"], name#"=">, HelpText;
+  def : Separate<["--", "-"], name>, Alias(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 ArgumentsAdjustingCompilations::getCompileCommands(
-StringRef FilePath) const {
+std::vector
+ArgumentsAdjustingCompilations::getC