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));
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122663/new/
https://reviews.llvm.org/D122663
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
