[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added inline comments.


================
Comment at: clang/include/clang/AST/ExprConcepts.h:502
                ArrayRef<ParmVarDecl *> LocalParameters,
+               SourceLocation RParenLoc,
                ArrayRef<concepts::Requirement *> Requirements,
----------------
rsmith wrote:
> erichkeane wrote:
> > Is this an unrelated change?  Are paren locations required for mangling?
> `requires (params) { blah; }` has a different mangling from `requires { blah; }` -- in particular, the former introduces a new level of function parameters, so references to enclosing function parameters are numbered differently in the two. This applies even if `params` is empty or `void` (`requires () { ... }` and `requires (void) { ... }` are both valid).
> 
> We previously generated the same AST for `requires { ... }` and `requires () { ... }`, which meant we couldn't mangle this correctly. Tracking the parens (or at least their existence) fixes that (and also improves the AST fidelity for tooling).
>>Tracking the parens (or at least their existence) fixes tha
it seems you test that via the `LParenLoc`, which is why I'm surprised about the addition of the `RParenLoc`?  I have no problem with the addition of the `RParenLoc` for the below reason, but was trying to figure out the significance toward the mangling.

>>and also improves the AST fidelity for tooling
This I definitely get, was just trying to figure out why it was in a patch for mangling.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147655