This is an archive of the discontinued LLVM Phabricator instance.

Implement P2173 for attributes on lambdas
ClosedPublic

Authored by aaron.ballman on Jan 29 2021, 11:39 AM.

Details

Summary

https://wg21.link/P2173 is making its way through WG21 currently and has not been formally adopted yet. This feature provides very useful functionality in that you can specify attributes on the various function *declarations* generated by a lambda expression, where the current C++ grammar only allows attributes which apply to the various function *types* so generated.

This patch implements P2173 on the assumption that it will be adopted by WG21 with this syntax because there are no other syntactic locations to put an attribute on a lambda that will unambiguously appertain to the declaration rather than the type. Given the large number of function declaration attributes compared to function type attributes, such a construct is needed. The proposal has been approved in principle by EWG and is scheduled to go through Core wording review, so there is a strong likelihood it will be adopted, at least with this syntax. When WG21 adopts the proposal, the diagnostic can be changed as appropriate. In the unlikely event that WG21 decides not to adopt the proposal, this syntax is still a conforming extension.

I need this functionality in a downstream fork of Clang that is currently sliding some attributes written in the type position to instead apply to the declaration.

Diff Detail

Event Timeline

aaron.ballman requested review of this revision.Jan 29 2021, 11:39 AM
aaron.ballman created this revision.

The patch seems technically okay to me. Do we need to recognize lambdas in any tentative-parse situations, or is it always the reverse (e.g. recognizing ObjC message sends as not-a-lambda)?

The warning is a bit weird. If we don't think it's certain that the committee will adopt this syntax, I don't think we should add this patch at all; it is not really acceptable to add it and then treat it as a Clang extension if the committee rejects it. If we do think it's certain, we should go ahead and consider this a feature of the next major standard.

rsmith added a comment.Feb 3 2021, 3:53 PM

The patch seems technically okay to me. Do we need to recognize lambdas in any tentative-parse situations, or is it always the reverse (e.g. recognizing ObjC message sends as not-a-lambda)?

Disambiguation with ObjC message sends never looks past the ] of the lambda introducer, so I think that's fine. And the disambiguation for [[ won't kick in here, because we've already committed to the [...] [[ introducing a lambda before we parse the attributes. The disambiguation for int a[] = { [n] = 0 }; also looks OK: it looks for an = after the ], which we can still use to disambiguate as an array designator rather than a lambda.

I think the error recovery disambiguation logic for delete [] { return p; } (); won't work if the lambda has attributes, but that seems fine: that's a best-effort recovery anyway.

The warning is a bit weird. If we don't think it's certain that the committee will adopt this syntax, I don't think we should add this patch at all; it is not really acceptable to add it and then treat it as a Clang extension if the committee rejects it. If we do think it's certain, we should go ahead and consider this a feature of the next major standard.

I think it's quite unlikely that the committee would reject the feature at this stage. Seems OK to me to jump the gun slightly and call this a C++23 extension.

The warning is a bit weird. If we don't think it's certain that the committee will adopt this syntax, I don't think we should add this patch at all; it is not really acceptable to add it and then treat it as a Clang extension if the committee rejects it. If we do think it's certain, we should go ahead and consider this a feature of the next major standard.

I think it's quite unlikely that the committee would reject the feature at this stage. Seems OK to me to jump the gun slightly and call this a C++23 extension.

SGTM, then.

aaron.ballman set the repository for this revision to rG LLVM Github Monorepo.

Updating based on review feedback.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2021, 8:57 AM

The warning is a bit weird. If we don't think it's certain that the committee will adopt this syntax, I don't think we should add this patch at all; it is not really acceptable to add it and then treat it as a Clang extension if the committee rejects it. If we do think it's certain, we should go ahead and consider this a feature of the next major standard.

I think it's quite unlikely that the committee would reject the feature at this stage. Seems OK to me to jump the gun slightly and call this a C++23 extension.

SGTM, then.

That works for me as well -- I'd also be very surprised if the committee rejected the feature at this point. When I originally worked on the patch, the paper hadn't started its polling in Evolution yet and so it was less clear how it would be received.

rjmccall added inline comments.Feb 4 2021, 10:30 AM
clang/include/clang/Basic/DiagnosticGroups.td
269 ↗(On Diff #321465)

Uh, I think we're a couple standard releases past the point at which we should have reconsidered this schema. I guess the problem is that we can't say -Wpre-c++23-compat without jumping the gun. Is there a problem with -Wc++20-compat and then having the earlier warning groups imply the later ones? That seems to be what we do with -Wc++98-compat; did we abandon that approach intentionally?

aaron.ballman added inline comments.Feb 4 2021, 10:39 AM
clang/include/clang/Basic/DiagnosticGroups.td
269 ↗(On Diff #321465)

@rsmith may have more background here. I was following the pattern already in the file, but I tend to agree that this pattern is not leading us somewhere good. FWIW, I ran into a similar situation with this on the C side of things in D95396, so we should probably be consistent there too.

Quuxplusone added inline comments.
clang/include/clang/Basic/DiagnosticGroups.td
269 ↗(On Diff #321465)

My understanding is that the command-line user is expected to pass

  • clang++ -std=c++20 -Wc++11-compat to indicate "I want actually to compile in C++20 mode, but give me warnings about anything that would prevent compiling in C++11 mode"
  • clang++ -std=c++17 -Wc++14-compat to indicate "I want actually to compile in C++17 mode, but give me warnings about anything that would prevent compiling in C++14 mode"
  • clang++ -std=c++14 -Wc++20-compat to indicate "I want actually to compile in C++14 mode, but give me warnings about anything that would prevent compiling in C++20 mode" — EXCEPT that I think this is not supported. My impression is that forward-compatibility warnings are generally just rolled into -Wall and not handled separately beyond that?

I don't think any human user is expected to pass -Wc++98-c++11-c++14-c++17-c++20-compat by hand; it's just an internal name for a particular subset of -Wc++98-compat.

IOW, we could choose a new naming scheme for it, but that would be a purely internal change that won't affect how command-line users interact with Clang at all (for better and for worse).

rjmccall added inline comments.Feb 4 2021, 11:41 AM
clang/include/clang/Basic/DiagnosticGroups.td
269 ↗(On Diff #321465)

Diagnostic groups can both directly contain diagnostics and imply other diagnostic groups, so I don't think there's any reason to make a dedicated group just to contain the new diagnostics in e.g. -Wc++14-compat except to allow someone turn on those warnings separately. And it does show up to users as the warning group under -fdiagnostics-show-option (which is the default).

rsmith added inline comments.Feb 4 2021, 2:56 PM
clang/include/clang/Basic/DiagnosticGroups.td
269 ↗(On Diff #321465)

@Quuxplusone's comment describes the intent. -std=c++20 -Wc++14-compat should give a more or less complete list of reasons why the code would not compile in C++14 (at least on the language side; we don't check for stdlib compatibility). The other direction -- -std=c++11 -Wc++14-compat -- is more of a best-effort check for things that we've seen cause problems in practice and can easily detect. (As a consequence, I don't think there's any subset/superset relation between -Wc++X-compat and -Wc++Y-compat.)

I'd be happy to see these groups renamed to -Wpre-c++20-compat or similar. Warning group synonyms are relatively cheap, so I wouldn't be worried about adding a -Wpre-c++2b-compat now and renaming it to -Wpre-c++23-compat flag later.

(As an aside, it'd be handy if there were some way to mark a DiagGroup as existing only for grouping purposes, so that we could avoid exposing a -W flag for cases where groups are added for internal reasons.)

1023 ↗(On Diff #321465)

I think we generally use a lowercase letter here, so CXX2b. (For example, see the C2x group.)

rjmccall added inline comments.Feb 4 2021, 4:15 PM
clang/include/clang/Basic/DiagnosticGroups.td
269 ↗(On Diff #321465)

Okay. It looks like -Wc++X-compat is consistently (1) all the this-feature-used-not-to-exist diagnostics from C++X and later plus (2) warnings about deprecation and semantic changes introduced by exactly version X. This seems like an unfortunate pairing, basically caused by the option names not being very clear about what kind of compatibility they mean. If we want @Quuxplusone's interpretation, which I agree is a natural human interpretation of those command lines, we'll need special support for it in diagnostic-option handling, so that we include specific diagnostics based on the relationship between the option and the language version.

There is a natural subset relationship between the this-feature-used-not-to-exist groups; we're just not taking advantage of it at all.

rsmith added inline comments.Feb 4 2021, 5:41 PM
clang/include/clang/Basic/DiagnosticGroups.td
269 ↗(On Diff #321465)

(2) sounds like a bug. Maybe we should add CXXPostXYCompat groups, symmetric to the CXXPreXYCompat groups, to better handle that?

I'm not sure about the need for special support in diagnostic option handling -- we don't ever produce a "you're using a feature that wasn't in an older standard mode" warning unless we're in the newer mode, and we don't ever produce a "you're using a feature that will change / go away in a newer standard mode" warning unless we're in the older mode.

I think it'd be reasonable to take advantage of the subset relationships. Back when there were only a couple of C++ language standards we cared about, the difference between linear and quadratic growth didn't really matter, but we're past that point now.

aaron.ballman added inline comments.Feb 5 2021, 6:42 AM
clang/include/clang/Basic/DiagnosticGroups.td
269 ↗(On Diff #321465)

In terms of what's reasonable for this patch, what's our path forward? It sounds like we'd like to see CXXPre2bCompat that's spelled -Wpre-c++2b-compat (and same for pedantic), and then we'll add aliases for the other language standard modes in a follow-up?

Updated based on review feedback.

Updated based on review feedback.

Ping

Updated based on review feedback.

Ping

Ping

Sorry. I have no problem with continuing the existing pattern, I guess.

rsmith accepted this revision.Mar 2 2021, 3:16 PM
rsmith added inline comments.
clang/include/clang/Basic/DiagnosticGroups.td
269 ↗(On Diff #321465)

I'm happy with what you have here, so long as the cleanup is actually done. I don't think this inconsistent state is OK for the longer term.

This revision is now accepted and ready to land.Mar 2 2021, 3:16 PM
aaron.ballman closed this revision.Mar 3 2021, 7:06 AM

I've commit in b2bc0a32545f9b066fed1631c6fba92a2a5a6d84 and will work on a patch for the remainder of the diagnostic groups. Thanks for the reviews!