[PATCH] D122663: Mark identifier prefixes as substitutable

[Nathan Sidwell via Phabricator via cfe-commits]

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