[PATCH] D95487: Itanium Mangling: Fix handling of <expr-primary> in <template-arg>.

[James Y Knight via Phabricator via cfe-commits]

jyknight added inline comments.


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:3912
+    IsPrimaryExpr = false;
+  };
+
----------------
jyknight wrote:
> rjmccall wrote:
> > I think it might be more reasonable to just check for the relatively small number of primary-expression cases in `mangleTemplateArgExpr` and skip the `X...E` there rather than pushing it down into this function.
> I started out writing it that way, but it's more complex than it first seems.
> 
> First you have the 9 expression types that always fit into <expr-primary> -- those are simple enough, but that's already a lot more than one might think. But, then you also need to handle recusing on all the cases with no output (ParenExprClass etc. -- 10 cases of this). And, finally, you have complex cases like DeclRefExpr, CXXConstructExprClass, and UnaryExprOrTypeTraitExprClass, which sometimes emit a primary expression and sometimes do not.
> 
> Putting all of that together, the number of cases to handle with a separate function was large enough -- and duplicative enough -- that it seemed more confusing to have such an implementation split than it was helpful.
Ultimately, we'd end up with a second large switch in a second function -- but it must be carefully kept in sync with any changes to the switch in mangleExpression. I think that'd just end up more likely to cause problems in the future, when new expression types are added.

Additionally, the NotPrimaryExpr idiom is already in use in mangleValueInTemplateArg, and doing the same thing for both cases is helpful for understandability, IMO.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95487/new/

https://reviews.llvm.org/D95487