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