[clang] [llvm] [clang][LLVM Demangler] Add an assertion that validates that all mang… (PR #111391)

[Erich Keane via cfe-commits]

erichkeane wrote:

> Sorry for jumping in late.
> 
> I think Erich is on point that having a flag that controls an assertion is a bit of a red flag as we are mixing build configuration and runtime configuration.
> 
> It is at least unusual and may cause confusion.
> 
> After thinking about this a bit more, should we maybe go all-in on one of the approaches?
> 
>     * either use build configuration: add a new build flag that controls this assertion. Only assert when assertions are enabled and the new build flag is defined.
> 
>     * or use runtime configuration always, e.g. add diagnostics for names that can't be demangled. It should make finding those issues much easier (one can run the compiler to produce a list of names that can't be demangled with locations pointing at code that helps to identify where those names come from).
> 
> 
> I would probably vouch for the second approach. The only downside that I see is that we have to expose the warning flag to users (right?) and this is something that should be internal (-cc1).
> 
> What do people think of that proposal?

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.

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