[PATCH] D122663: Mark identifier prefixes as substitutable

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:6031
+bool CXXNameMangler::mangleSubstitution(NestedNameSpecifier *NNS) {
+  NNS = Context.getASTContext().getCanonicalNestedNameSpecifier(NNS);
+  return mangleSubstitution(reinterpret_cast<uintptr_t>(NNS));
----------------
This seems a little error-prone to me: calling this on a type NNS would do the wrong thing (those are supposed to share a substitution number with the type, rather than have a substitution of their own).

We could handle the various cases here and dispatch to the right forms of `mangleSubstitution` depending on the kind of NNS, but that code would all be unreachable / untested. So maybe we should just make this assert that `NNS->getKind() == NestedNameSpecifier::Identifier`. (And optionally we could give this a more specific name, eg `mangleSubstitutionForIdentifierNNS`?)


================
Comment at: clang/test/CodeGenCXX/clang-abi-compat.cpp:160
+template <typename T> void test10(typename T::Y::a, typename T::Y::b) {}
+// PRE15: @_Z6test10I1XEvNT_1Y1aENS1_1Y1bE
+// V15:   @_Z6test10I1XEvNT_1Y1aENS2_1bE
----------------
I think this test does not capture an important property for which we should have test coverage: that we used to not register a substitution for `T::Y` (not only that we used to not *use* a substitution for it). For example, this test would capture that:

```
struct X {
  struct Y {
    using a = int;
    using b = int;
  };
};
template <typename T> void test10(typename T::Y::a, typename T::Y::b, float*, float*) {}
template void test10<X>(int, int, float*, float*);
```
... where the numbering of `float*` depends on how many substitutions we created for the earlier types.

(Put another way, this test covers only one of the two `if (!Clang14Compat)` tests in ItaniumMangle.cpp, and we should cover both.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122663