[PATCH] D122663: Mark identifier prefixes as substitutable

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk marked 2 inline comments as done.
hvdijk 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));
----------------
rsmith wrote:
> 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`?)
I have added the assert that you suggested. I would actually have preferred for this function to be used for other NNS substitutions as well to better align with how the spec says substitutions should be handled (it's a rule of `<prefix>`, not any component within) but that seemed like an unnecessarily more invasive change. If you are okay with it I would like to keep the function named just `mangleSubstitution` to keep that open as option for a possible future clean-up.


================
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
----------------
rsmith wrote:
> 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.)
I get where you're coming from. My thinking was the other way around, if we add the first `if (!Clang14Compat && ...)` by itself that will have no effect and cannot be tested for, then if we add the second `if (!Clang14Compat)` that does have impact and needs testing. You're looking at it the other way around, if we add the second `if (!Clang14Compat)` first, we fix one bug, and if we then add the first `if (!Clang14Compat && ...)` we fix another. Happy to extend the test like you suggested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122663