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