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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
