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

Sam McCall via cfe-commits cfe-commits at lists.llvm.org
Thu Apr 18 05:54:18 PDT 2024


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<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.

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


More information about the cfe-commits mailing list