[PATCH] D133659: [Clang] P1169R4: static operator()

[Roy Jacobson via Phabricator via cfe-commits]

royjacobson marked 2 inline comments as done.
royjacobson added a comment.

Wow, thanks for the quick review!

In D133659#3783008 <https://reviews.llvm.org/D133659#3783008>, @cor3ntin wrote:

> 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 :)

> - classes having both static and non-static `operator()` . especially the example in https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p1169r4.html#overload-resolution deserves a test.

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.



================
Comment at: clang/lib/Sema/SemaLambda.cpp:954-955
     //   by mutable. It is neither virtual nor declared volatile. [...]
-    if (!FTI.hasMutableQualifier()) {
+    if (!FTI.hasMutableQualifier() &&
+        ParamInfo.getDeclSpec().getStorageClassSpec() != DeclSpec::SCS_static) {
       FTI.getOrCreateMethodQualifiers().SetTypeQual(DeclSpec::TQ_const,
----------------
cor3ntin wrote:
> Do we actually need that check ? It should be diagnosed already.
It's not about diagnosing, it's preventing declaring the operator 'const' if it's static.



================
Comment at: clang/test/CXX/over/over.oper/over.literal/p7.cpp:23-25
+struct Functor {
+  static int operator()(int a, int b);  // cxx11-error {{cannot be a static member function}}
+};
----------------
cor3ntin wrote:
> 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?
Oops, this is the wrong p7 indeed :)

thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133659