This is an archive of the discontinued LLVM Phabricator instance.

[Clang] P1169R4: static operator()
ClosedPublic

Authored by royjacobson on Sep 11 2022, 12:58 AM.

Details

Summary

Implements 'P1169R4: static operator()' from C++2b.

Diff Detail

Event Timeline

royjacobson created this revision.Sep 11 2022, 12:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 11 2022, 12:58 AM

missing newline

Fix the tests, add a CodeGen test.

Fix diagnosing static operators that are not call.

royjacobson published this revision for review.Sep 11 2022, 4:56 AM
royjacobson retitled this revision from [Clang] Implement static operator() to [Clang] P1169R4: static operator().
royjacobson edited the summary of this revision. (Show Details)
royjacobson added a reviewer: Restricted Project.
Herald added a project: Restricted Project. · View Herald TranscriptSep 11 2022, 4:56 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
royjacobson edited the summary of this revision. (Show Details)

Fix codegen test on windows, rebase on main.

Thanks a lot for working on that!

I think this is the right direction but i would like to see more tests.
Notably:

There is the question of whether we'd want the call operator to be implicitly static but I'm don't think it's currently allowed, it may have abi impact and it doesn't to be dealt with in this PR so that should not hold us.
The other question is whether we want to make static operator() an extension in older language modes. I don;t see a reason to prohibit that (with a warning)

clang/include/clang/Basic/DiagnosticParseKinds.td
1051
clang/include/clang/Basic/DiagnosticSemaKinds.td
9080

For consistency and clarity, I'd say 'static overloaded %0'

9082
clang/lib/Parse/ParseExprCXX.cpp
1265

We might want to add a note showing where the capture list is.

clang/lib/Sema/SemaChecking.cpp
5856
clang/lib/Sema/SemaLambda.cpp
963–964

Do we actually need that check ? It should be diagnosed already.

995–997

Do we want an assert to make sure it's not something else than static or none?

clang/test/CXX/over/over.oper/over.literal/p7.cpp
23–25 ↗(On Diff #459361)

This doesn't really seem to be the right file for this test - I'd expect that to be in a test file where we do sema check on operators, not literal operators.
Do you need help looking for a better place?

royjacobson marked 2 inline comments as done.Sep 11 2022, 12:07 PM

Wow, thanks for the quick review!

Thanks a lot for working on that!

I think this is the right direction but i would like to see more tests.
Notably:

  • converting a lambda with a static operator to a pointer

Turns out I had a bug in the conversion function generation, thanks for the suggestion! Fixed it and added tests.

  • tests in dependant contexts

Added some tests for that, but I'm not sure how to judge if I have good coverage of all the possible edge cases, so more suggestions are welcome :)

I _think_ you can't do that - https://stackoverflow.com/questions/5365689/c-overload-static-function-with-non-static-function.

The example from the paper is about disambiguating the conversion-to-function-pointer call from the operator(), which is kind-of already tested because otherwise you wouldn't be able to call a static lambda at all. I added it as an explicit test, though.

  • unevaluated lambda tests / constexpr tests

I'm not sure I understand what you mean by unevaluated lambdas, could you elaborate? I added some constexpr/eval tests.

There is the question of whether we'd want the call operator to be implicitly static but I'm don't think it's currently allowed, it may have abi impact and it doesn't to be dealt with in this PR so that should not hold us.
The other question is whether we want to make static operator() an extension in older language modes. I don;t see a reason to prohibit that (with a warning)

I thought I made it an extension, but apparently it was only for the static lambdas, just because that's how most of the other lambda extensions were implemented :)
I don't have a strong opinion about this. But I realized that without backporting the overload resolution changes, this will not be useful for lambdas.
So just to be consistent right now I made it an error for both - we can always change that to a warning pretty easily.

clang/lib/Sema/SemaLambda.cpp
963–964

It's not about diagnosing, it's preventing declaring the operator 'const' if it's static.

clang/test/CXX/over/over.oper/over.literal/p7.cpp
23–25 ↗(On Diff #459361)

Oops, this is the wrong p7 indeed :)

thanks!

royjacobson marked an inline comment as done.

Fix conversion to function pointer, add more tests and apply Corentin's wording.

Fix test after warning text change

royjacobson marked 6 inline comments as done.Sep 12 2022, 11:18 AM
royjacobson added inline comments.
clang/lib/Parse/ParseExprCXX.cpp
1265

I added a note, but the captures list is always just before the static specifier, so I'm not sure how useful it is. WDYT?

royjacobson marked an inline comment as done.

Add note in diagnostic.

lichray added inline comments.
clang/test/CodeGenCXX/cxx2b-static-call-operator.cpp
6

I want to see some tests that diagnose extern operator().

clang/test/Parser/cxx2b-lambdas.cpp
43

And some tests that diagnose extern, mutable extern, etc.

shafik added a subscriber: shafik.Sep 12 2022, 5:38 PM
shafik added inline comments.
clang/lib/Sema/SemaDeclCXX.cpp
15934
clang/test/Parser/cxx2b-lambdas.cpp
42

also combined w/ constexpr and consteval

52

Why not throw in capture by copy/move as well.

Add tests required by reviewerd.

Beside a missing test, this LGTM
I'd like someone else (@erichkeane maybe) to review the codegen tests.

clang/test/Parser/cxx2b-lambdas.cpp
60

For completeness, can you add a test for *this/this?

Added tests for 'this' lambda captures and for the pre-cxx2b compatability warnings.

Does anyone feel comfortable with looking at the CodeGen tests? They're checking that we aren't passing a this argument.

@erichkeane @shafik @lichray

Does anyone feel comfortable with looking at the CodeGen tests? They're checking that we aren't passing a this argument.

@erichkeane @shafik @lichray

The codegen tests look reasonably complete and correct to me.

Thanks everyone! So if no one else has comments I'm planning to merge this tomorrow.

aaron.ballman added a subscriber: aaron.ballman.

Thanks everyone! So if no one else has comments I'm planning to merge this tomorrow.

FWIW, you shouldn't merge something that has nobody signed on as the reviewer (and has only mild acceptance from people subscribing) -- a better approach is to add some reviewers, get them to accept, then land. Otherwise, coming back to this review in the future when doing code archeology leads to confusion about whether the changes should have even gone in at all, etc. I've added some more reviewers so we can make sure this gets the pretty green checkmark before landing. :-)

clang/include/clang/Basic/DiagnosticParseKinds.td
1043

This is a bit confusing -- we're saying it's an extension but then making it an error? I've reworded based on how I think you're intending it to be issued.

1047–1051

These are semantic errors, not parsing ones. This means these will be diagnosed when parsing the lambda rather than when instantiating it. I don't think that matters for the cast of combining mutable and static, but I'm less certain about "have any captures" because of cases like:

template <typename... Types>
auto func(Types... Ts) {
  return [Ts...] { return 1; };
}

int main() {
  auto lambda = func();
}

I'm pretty sure that lambda has no captures for that call, but it could have captures depending on the instantiation.

Actually, from some off-list discussion with @erichkeane, even mutable and static are a problem in code like:

template <typename Ty>
void func(T t) {
  if constexpr (Something<T>) {
    [](){};
  } else {
    [t](){};
  }

where the lambda is in a discarded statement.

So I think these might need to change to be Sema diagnostics (and we should add some additional test coverage).

clang/include/clang/Basic/DiagnosticSemaKinds.td
9080
clang/lib/Sema/SemaDeclCXX.cpp
15939–15945

The diagnostic formatter knows how to format anything that inherits from NamedDecl.

royjacobson added inline comments.Sep 22 2022, 7:21 AM
clang/include/clang/Basic/DiagnosticParseKinds.td
1047–1051

From https://eel.is/c++draft/expr.prim.lambda.general#4

If the lambda-specifier-seq contains static, there shall be no lambda-capture

So this should be a parsing error. Or maybe I don't understand what you're saying. There are no static lambdas in your examples so I'm not sure how they're related.

aaron.ballman added inline comments.Sep 22 2022, 7:36 AM
clang/include/clang/Basic/DiagnosticParseKinds.td
1047–1051

So this should be a parsing error. Or maybe I don't understand what you're saying.

Parsing errors are where the grammar disallows something, generally. The rest are semantic diagnostics (e.g., we can parse the construct just fine, but we diagnose when turning it into an AST node because that's the point at which we have complete information about what we've parsed).

That said, my concern was mostly around SFINAE situations. My recollection is that SFINAE traps do not cover parsing errors only type substitution errors. So for my first example, I would expect there to be no parsing error despite specifying a capture list because that capture list can be empty when the pack is empty, but we would get a SFINAE diagnostic when rebuilding declaration during template instantiation if the pack was not empty.

There are no static lambdas in your examples so I'm not sure how they're related.

Sorry, I was being lazy with my examples and showing more about the capture list. Consider:

template <typename... Types>
auto func(Types... Ts) {
  return [Ts...] static { return 1; };
}

int main() {
  auto lambda = func();
}
royjacobson added inline comments.Sep 22 2022, 7:57 AM
clang/include/clang/Basic/DiagnosticParseKinds.td
1047–1051

Like I said,

If the lambda-specifier-seq contains static, there shall be no lambda-capture

Ts... is still a syntactic lambda-capture, even if it's instantiated as an empty pack. I don't see how that's not a parsing error.

aaron.ballman added inline comments.
clang/include/clang/Basic/DiagnosticParseKinds.td
1047–1051

Ah, I think I maybe see where the confusion is coming in, now: you think my example should be diagnosed and I think my example should be accepted.

Based on the standards wording, I think you're right. The standard specifically uses "lambda-capture" as a grammar term, so it *is* a parsing error at that point.

Based on my understanding of the intent behind the feature, I think I'm right and there's a core issue here. I don't think we intended to prohibit empty packs from being captured as non-SFINAEable error (and in off-list talks with @erichkeane, he agrees). as that allows different specializations of the function containing the lambda, which could be of use. However, I'm not 100% sure on the intent. CC @hubert.reinterpretcast as the C++ standards conformance code owner to see if he has an opinion or other recollections here.

Apply suggestions from Aaron and rebase.

royjacobson marked 8 inline comments as done.

Small doc change

royjacobson marked an inline comment as not done.Sep 23 2022, 4:14 AM
royjacobson added inline comments.
clang/include/clang/Basic/DiagnosticParseKinds.td
1047–1051

I can see something like

[Ts...] static(sizeof...(Types) == 0) {}

being eventually useful, but as long as the static is non-conditional allowing a capture list with an empty pack doesn't make sense to me. I think you can always do something like

[]<typename = std::enable_if<sizeof...(Types) == 0>::value> {}

and that should have the same functionality.

Do you or Erich plan to open a CWG issue about this?

cor3ntin added inline comments.Sep 23 2022, 4:18 AM
clang/include/clang/Basic/DiagnosticParseKinds.td
1047–1051

FYI I agree with @royjacobson on the current wording interpretation.
I don't think sfinae on empty pack offer much benefit - versus the complexity (and the cost of not being able to diagnose non-empty packs until instanciations) so I don't think there is a core issue here either. But until such core issue emerges and is resolved, I don't think we need to change what is done here.

aaron.ballman added inline comments.Sep 23 2022, 6:20 AM
clang/include/clang/Basic/DiagnosticParseKinds.td
1047–1051

Do you or Erich plan to open a CWG issue about this?

I was contemplating it, but after thinking on it more overnight, I think we should accept this patch as-is. I believe it would be safe and plausible to lift this restriction later if CWG or EWG decides to move in this space, but the amount of utility gained by supporting it now is pretty minimal.

Thanks for the discussion around this!

clang/lib/Frontend/InitPreprocessor.cpp
698

Because we're exposing the feature as a language extension in older language modes, I think we probably want to define the macro in older language modes as well, right?

clang/lib/Parse/ParseExprCXX.cpp
1174

coding style nit.

1265

FWIW, I'm not sure the note adds a whole lot of value. @cor3ntin, do you have a code example in mind where you think the note would be clarifying?

clang/lib/Sema/SemaDeclCXX.cpp
15391–15393
clang/test/Parser/cxx2b-lambdas-ext-warns.cpp
2–4

You weren't using the expected prefix in the test.

royjacobson marked 7 inline comments as done.

Apply small suggestions by Aaron

royjacobson planned changes to this revision.Sep 24 2022, 10:02 AM

I think I didn't get the overload resolution changes quite right, so this will probably change a bit.

clang/lib/Frontend/InitPreprocessor.cpp
698

Because supporting static lambdas requires backporting the overload resolution change as well, I decided not to support it as an extension, and for consistency no regular static operator() as well.

I've asked now on the libcxx Discord and they seem to think that it would still be useful to provide the regular static operator() as an extension, especially for C++20 ranges.

I think backporting the overload resolution change should be fine, but it seems I haven't implemented it quite right so I'm looking into it.

  • Fix overload resolution and apply it retroactively
  • Allow static lambdas and static operator() as extensions
  • Define the feature macros in earlier versions and fix the tests

GCC have implemented this yesterday, and they also apply the overload resolution change retroactively. So I've done it as well and downgraded the errors to ExtWarns.

I'm not sure that the change to the conversion sequences in the overload resolution is the best way to implement this but it was the cleanest way I found..

I only left a few comments because I'm not familiar enough with that part of the code to be certain the changes are correct but overall it looks fine to me.

clang/include/clang/Sema/Overload.h
524–526 ↗(On Diff #463346)
clang/lib/Sema/SemaOverload.cpp
7024
7027

Can we move that outside of the scope to avoid duplication?

Rebase and address comments.

royjacobson marked 3 inline comments as done.Sep 29 2022, 8:46 AM
cor3ntin added a comment.EditedSep 29 2022, 8:58 AM

@aaron.ballman Can you give one last pass at this? The new conversions rules is not something I feel confident blessing (ie I'm not qualified).

Rebase (main was broken)

Double-checking my understanding of the overload resolution changes: we added a new conversion sequence, but we don't expect that conversion sequence to cause a change in overload resolution in practice? (I'm wondering if there's test coverage we should be adding for those changes.)

clang/lib/Parse/ParseExprCXX.cpp
1265

I think we should remove the note here -- we can add a note back when we run into a use case where it would be enlightening.

clang/lib/Sema/SemaOverload.cpp
7028–7030

Can you replace this with: Candidate.Conversions[FirstConvIdx].setStaticObjectArgument(); and get the same behavior?

clang/test/Lexer/cxx-features.cpp
42–48 ↗(On Diff #463926)

Unrelated changes? (Feel free to land as an NFC change separately though, they look like sensible changes.)

clang/test/SemaCXX/lambda-unevaluated.cpp
125

We can remove this now, right?

cor3ntin added inline comments.Sep 29 2022, 10:36 AM
clang/lib/Parse/ParseExprCXX.cpp
1265

I was just thinking that with sufficient attributes, templates, require clauses and parameters, the lambda might be cluttered enough that a note would not hurt. I'm not strongly attached to it though

royjacobson marked 7 inline comments as done.

Address CR comments

Double-checking my understanding of the overload resolution changes: we added a new conversion sequence, but we don't expect that conversion sequence to cause a change in overload resolution in practice? (I'm wondering if there's test coverage we should be adding for those changes.)

Yeah. The change only allow previously ambiguous calls to now not be ambiguous. Add to that that static and non-static methods can't overload each other, and the fact that GCC have applied this retroactively as well, I think it's pretty safe to just use the usual regression tests. (https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=303976a6076f2839354702fd2caa049fa7cbbdc2)

aaron.ballman accepted this revision.Sep 29 2022, 12:06 PM

LGTM, thank you for all the work on this!

This revision is now accepted and ready to land.Sep 29 2022, 12:06 PM

Thanks for the patient review and the discussion! @aaron.ballman @cor3ntin

This revision was automatically updated to reflect the committed changes.