Page MenuHomePhabricator

Implemented P0409R2 - Allow lambda capture [=, this]
ClosedPublic

Authored by hamzasood on Aug 10 2017, 4:19 AM.

Details

Summary

This patch implements P0409R2.

'*this' capture is allowed pre-C++17 as an extension. So I've also allowed [=, this] pre-C++2a as an extension (with appropriate warnings) for consistency.

Diff Detail

Event Timeline

hamzasood created this revision.Aug 10 2017, 4:19 AM
rjmccall added inline comments.Aug 10 2017, 1:15 PM
test/FixIt/fixit-cxx0x.cpp
57

Shouldn't you only be accepting this in C++2a mode?

hamzasood added inline comments.Aug 10 2017, 1:30 PM
test/FixIt/fixit-cxx0x.cpp
57

I'm not sure what the system is with allowing future language features as extensions, but I noticed that [*this] capture is allowed as an extension pre-C++17 so I figured it would make sense for [=, this] to also be allowed as an extension (since the proposal mentions how it's meant to increase code clarify in the presence of [*this]).

rjmccall added inline comments.Aug 10 2017, 1:55 PM
test/FixIt/fixit-cxx0x.cpp
57

Surely there should at least be an on-by-default extension warning? The behavior we're using sounds a lot more like we're treating this as a bug-fix in the standard than a new feature. Richard, can you weigh in here?

hamzasood added inline comments.Aug 10 2017, 2:03 PM
test/FixIt/fixit-cxx0x.cpp
57

The extension warning for this (ext_equals_this_lambda_capture_cxx2a) is on by default.

rjmccall added inline comments.Aug 10 2017, 5:35 PM
test/FixIt/fixit-cxx0x.cpp
57

Why did the diagnostic disappear from this file, then?

hamzasood added inline comments.Aug 11 2017, 2:45 AM
test/FixIt/fixit-cxx0x.cpp
57

That file is for FixIt hints, which I don't think make much sense for an extension warning (and I couldn't find any other extension warnings that offer FixIt hints)

rjmccall added inline comments.Aug 11 2017, 10:46 AM
test/FixIt/fixit-cxx0x.cpp
57

Sure, it's reasonable for this specific test to not test the warning. However, since I don't see anything in this test that actually turns off the warning, and since you have not in fact added any tests that verify that the warning is ever turned on, I suspect that it is actually not being emitted.

@rjmccall The warning is emitted, but you're right that there's no test to ensure that actually happens; test/CXX/expr/expr.prim/expr.prim.lambda/p8.cpp used Wno-c++2a-extensions to suppress the warning.

I've changed it so that checking for the warning is now part of the test.

faisalv added inline comments.Aug 13 2017, 7:47 AM
lib/Sema/SemaLambda.cpp
959

Shouldn't we try and hit the 'continue' (that u deleted) if warnings (and extension warnings) are turned into errors?

test/SemaCXX/cxx2a-lambda-equals-this.cpp
7

Nice - I wish we had (and that I had placed) more comments such as these in our test files :)

hamzasood added inline comments.Aug 13 2017, 8:52 AM
lib/Sema/SemaLambda.cpp
959

That's an interesting scenario which admittedly I hadn't considered.

I based this implementation on the '*this' capture handling from the same loop. When a '*this' capture is seen pre-C++1z, an extension warning is emitted and then the capture is processed as normal (i.e. without consideration of that warning potentially becoming an error).

I also looked at other places where extension warnings are emitted and I couldn't find any special handling for warnings becoming errors.

faisalv accepted this revision.Aug 13 2017, 9:39 AM

OK - looks good enough to me. I say we give the rest of the reviewers until friday to chime in, and if no one blocks it, can you commit then?
nice work - thanks!

This revision is now accepted and ready to land.Aug 13 2017, 9:39 AM
rjmccall added inline comments.Aug 14 2017, 10:56 AM
lib/Sema/SemaLambda.cpp
959

Yeah, I doubt there's a single place in the compiler where we stop processing code when warnings are turned into errors.

test/FixIt/fixit-cxx0x.cpp
57

I'm sorry, I see that you've added an explicit test for the warning, but I still don't understand why the warning is not emitted in this file. -verify normally verifies all diagnostics, and this file is tested with -std=c++11. Is there some special behavior of -fixit that disables warnings, or ignores them in -verify mode?

hamzasood added inline comments.Aug 14 2017, 11:13 AM
test/FixIt/fixit-cxx0x.cpp
57

Ah okay, now I know what you mean. The line you’re talking about has actually been removed completely from the fixit test.

rjmccall added inline comments.Aug 14 2017, 11:24 AM
test/FixIt/fixit-cxx0x.cpp
57

Oh, of course, I should've realized that right away. Sorry for the run-around.

In that case, this all LGTM.

hamzasood updated this revision to Diff 111681.Aug 18 2017, 9:03 AM

It's Friday, so hopefully this is good to go.
I don't have svn access, so could someone commit this for me?

I've updated the patch so that it can be applied successfully to the latest revision.
I've also renamed cxx1z->cxx17 in the diagnostic id to match @rsmith's recent patch.

faisalv closed this revision.Aug 18 2017, 8:45 PM

Committed as r311224, on behalf of Hamza.

https://reviews.llvm.org/rL311224