NagyDonat wrote:
In commit
https://github.com/llvm/llvm-project/pull/139256/commits/13f4a3011e76c5665ca295ef597eb130f356df68
I implemented your suggestion that the debug name of the checker name should
be derived from the name fragment of the registration functions (which is
called `CLASS` but may be different from the checker class name), but this
change had to touch yet another technically indebted area – the interface
provided for registering checkers from analyzer plugins – and became
surprisingly ugly.
I pushed this commit mostly to show its convoluted nature, I would strongly
prefer rejecting it, because it does not solve the fundamental issue (namely,
that the debug name of the checker family is something "taken from" one
subchecker and will change depending on which subcheckers are registered) and
introduces several additional complications:
- a few already complex methods become even more complicated because they need
to forward yet another argument;
- checkers that are registered from plugins won't have this kind of debug name;
- this way the debug name of the checker will _look like_ the class name, but
can be something else, which is IMO worse than something that is consistently
different.
If I sort the possible implementations of `CheckerFamily::getDebugName` in
order of increasing ugliness, then I get something like:
1. (least ugly) `= 0` and override it in each of the two dozen checker families.
- Provides a nice proper identifier (the class name).
- Boilerplate is boring, but not as bad as hacky or convoluted code.
- There is ample precedent for similar amounts of boilerplate (e.g. the
checker registration functions) and this would be much less boilerplate than
the old way of introducing multipart checkers (with all the `mutable
std::unique_ptr<BugType>`s).
- Would just add 20-30 oneline functions, which isn't significantly more
than a "general" solution.
- Can be easily replaced by a different solution later if we find a good
one (e.g. after reforming the structure of `Checkers.td`).
2. Use the class name in assertion-enabled builds on GCC, LLVM (from
`__PRETTY_FUNCTION__`) and MSVC (from `__FUNCSIG__`), and default to a filler
("UnknownCheckerFamily") on production builds and unknown compilers.
- Provides a nice proper identifier (the class name).
- Requires hacky magical code, but can be hidden away in a separate header
and won't complicate other stuff.
- Works out of the box for practically all developers of the static
analyzer.
- Won't break the build of LLVM/clang elsewhere.
3. Use the `CLASS` field of the checker specifications in `Checkers.td`, but
ensure that it is always identical to the class name (and the system can handle
multiple checkers with the same `CLASS`).
- Provides a nice proper identifier (the class name).
- Requires updating the information in `Checkers.td`.
- Requires touching the fragile "register checkers from analyzer plugins"
logic (as in
https://github.com/llvm/llvm-project/pull/139256/commits/13f4a3011e76c5665ca295ef597eb130f356df68)
- Currently the naming scheme of the checker registration function relies
on the fact that the `CLASS` field is a unique identifier that distinguishes
multiple checkers from the same class -- we would need to devise a different
solution for this.
4. Use "family-of-" + name of first registered subchecker, i.e.
`family-of-core.DivideZero`.
- Ugly, but not misleading identifiers: it is clear that this identifies a
family, and the checker name is searchable.
- Easy to implement.
- "Identifier" is not stable: depends on the set of currently registered
subcheckers (but at least its semantical meaning stays the same).
- Format may be bikeshedded: I'm also ok with `family:core.DivideZero` or
something similar.
5. Use name of first registered subchecker, i.e. plain `core.DivideZero`.
- Misleading, developers would naturally assume that it "obviously" refers
to the single checker part.
- Very easy to implement, shortest code that actually provides an
identifier.
- "Identifier" is not stable: depends on the set of currently registered
subcheckers.
6. Use the `CLASS` field of the checker specifications in `Checkers.td` (as
currently implemented by
https://github.com/llvm/llvm-project/pull/139256/commits/13f4a3011e76c5665ca295ef597eb130f356df68)
- Very misleading, looks like a nice proper identifier (the class name) but
may be different in rare cases.
- Requires touching the fragile "register checkers from analyzer plugins"
logic.
- "Identifier" is not stable: depends on the set of currently registered
subcheckers.
@steakhal What do you think about this list?
https://github.com/llvm/llvm-project/pull/139256
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
