[clang] [llvm] [clang][LLVM Demangler] Add an assertion that validates that all mang… (PR #111391)
Erich Keane via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 10 07:00:32 PDT 2024
erichkeane wrote:
> > I agree completely. I have a preference for the second thing as well. We can have this be a warning-as-default-error type thing, which allows us to disable this with a flag, but is still an error. I MIGHT suggest using the functionality to see if that diagnostic is on before doing the test (due to how much additional work there is here), but IMO, that is a much better way forward.
>
> 👍 I guess that's a good signal that this is the way to go. AFAIK, @VitaNuo has already started exploring this direction. Wrt to enabling this warning, I think we should probably start by having this warning disabled because there seems to be too many failures, but we could gradually roll it out:
>
> * start with a warning that's disabled by default,
>
> * collect issues that we hit in LLVM tests (there's plenty AFAIU) and also from real projects (we can run with the warning internally over Google's codebase to get a representative sample),
>
> * fix all demangling issues present in the tests and the most common ones from other projects,
>
> * enable the flag by default.
>
>
> @erichkeane how do you feel about having it enabled given that this does not really block users, but merely points at demangling issues in LLVM's demangler (that they don't necessarily even use). It feels like this should be internal, but then we won't get any bug reports. I feel like we should at least fix the majority of the issues before enabling it (hence the rough roadmap above).
I definitely have some heartburn about the enabling-of-flag by default, I would expect these diagnostics to be something actionable by our users, but I also definitely see the wish to get as many people testing this to improve the llvm demangler as possible. I pinged a few others to comment here, but I'd have to be reasonably sure this is a 'no one will ever see this' kind of thing, and if they DO, have a good idea of what to do.
I'm on the fence, but if we were to, I have some thoughts:
1- Can we test this on the top X github C++ projects as well? Perhaps at LEAST Boost.
2- Can we ensure that anyone who searches for the diagnostic has a way of VERY QUICKLY seeing how to report it? So it has to be VERY well documented.
3- Have the diagnostic make it clear that this isn't a "something wrong with your code, but with our demangler"?
4- Instead of being a warning, how about making it a 'remark'?
https://github.com/llvm/llvm-project/pull/111391
More information about the cfe-commits
mailing list