urnathan 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)); ---------------- hvdijk wrote: > urnathan wrote: > > hvdijk wrote: > > > 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. > > One could name it `mangleSubstitutionForIdentifierNNS` now and rename it in > > the future, if your unification dream comes true? That names it for what > > it does now. > > > > Just a thought, not a requirement. > Sorry, that comment came after I pushed it already. I have some more things > to look into when I have some more time (including @erichkeane's comment > about the test), will see if it makes sense to include with that, or perhaps > to just make that NFC change to allow it to be used more generally. no worries, I failed to notice it'd already landed 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