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
