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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
