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

[Richard Smith - zygoloid via Phabricator via libcxx-commits]

rsmith added a comment.

In D147655#4248800 <https://reviews.llvm.org/D147655#4248800>, @royjacobson wrote:

> I agree it doesn't affect too much code, but people do instantiate templates manually sometimes. IIUC, linking against shared libraries would break if that library does explicit instantiations of constrained functions. That concerns me a bit.

There has not been any stable ABI from any compiler targeting the Itanium C++ ABI for constrained templates prior to this change. I don't think we need to worry too much about people using unfinished compiler features being broken when those features are finished.

>From an ABI perspective, I think the other cases that I called out are more interesting than the constrained templates side of things: this changes the mangling for non-type template parameters with deduced types, non-type template parameters with instantiation-dependent types, and template template parameters that don't match their template template arguments, in the template parameter list of a function template. The latter two of those changes can in principle affect code dating back to C++98. There might be a case for putting those changes in particular behind a flag. @rjmccall I would particularly appreciate your feedback on that concern.

> Also, do you know if GCC developers are planning on implementing this change as well?

I have no reason to think they would not follow the cross-vendor ABI specification, though I'll ping them explicitly. The ABI proposals haven't been accepted yet; I'm not intending to land this change until the proposals have reached consensus in the Itanium C++ ABI group.



================
Comment at: clang/include/clang/AST/ExprConcepts.h:502
                ArrayRef<ParmVarDecl *> LocalParameters,
+               SourceLocation RParenLoc,
                ArrayRef<concepts::Requirement *> Requirements,
----------------
erichkeane wrote:
> 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.
Yeah, we don't really need to track the `RParenLoc`, or the `LParenLoc` for that matter, as part of this mangling change, we just need any way to get the "were there parens?" information.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147655