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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
