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:
> 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.
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
