[PATCH] D122663: Mark identifier prefixes as substitutable

Mon Apr 25 07:19:48 PDT 2022

hvdijk added a comment.

In D122663#3471763 <https://reviews.llvm.org/D122663#3471763>, @erichkeane wrote:

> I DID see that and am REASONABLY sure you do, but sometimes folks will ask for test coverage because they suspect the resulting behavior will demonstrate some sort of problem with the current patch.  I DID run this through the demangler and found that the PRE15 case looks odd?
>
> `void test10<X>(X::Y::a, X::Y::b, float*, X::Y)`
> vs
> `void test10<X>(X::Y::a, X::Y::b, float*, float*)`
>
> Or is that exactly the bug being caught here?  That is, is the `float*` substitution not being properly registered/emitted and being confused with `X::Y`?

Yes, that is exactly it. That is what @rsmith was getting at with his comment about regarding this as two separate bugs (he can correct me if I am misstating things), one that we do not add the substitution and as such use wrong numbers for subsequent substitutions, the other that we do not use the substitution. The other is what I had covered in my initial test, his addition covers that first bug where we wrongly used S4 when we should be using S5.

> PS:, and as an unrelated-lament... A while back I wrote a test regarding mangling where I ALSO had it run us through llvm-cxxfilt so that we had an output of every mangled string that showed the demangled version.  I wish we'd done that from the beginning to make it much less confusing what is going on in these.

llvm-cxxfilt has no option to be compatible with earlier incorrect mangling though, so it would not help with this particular test. But it could help with other tests, agreed.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122663/new/

https://reviews.llvm.org/D122663