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