Implemented P0409R2 - Allow lambda capture [=, this]

[hamzasood]

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.

[rjmccall]

test/FixIt/fixit-cxx0x.cpp

57

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

[hamzasood]

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]

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]

test/FixIt/fixit-cxx0x.cpp

57

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

[rjmccall]

test/FixIt/fixit-cxx0x.cpp

57

Why did the diagnostic disappear from this file, then?

[hamzasood]

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]

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.

[hamzasood]

@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]

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

6

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

[hamzasood]

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]

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!

[rjmccall]

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]

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]

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]

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]

Committed as r311224, on behalf of Hamza.

https://reviews.llvm.org/rL311224

include/clang/Basic/DiagnosticGroups.td

 def CXXPre1zCompat : DiagGroup<"c++98-c++11-c++14-compat">;
 def CXXPre1zCompatPedantic : DiagGroup<"c++98-c++11-c++14-compat-pedantic",
                                        [CXXPre1zCompat]>;
+def CXXPre2aCompat : DiagGroup<"c++98-c++11-c++14-c++1z-compat">;
+def CXXPre2aCompatPedantic : DiagGroup<"c++98-c++11-c++14-c++1z-compat-pedantic",
+                                       [CXXPre2aCompat]>;
 
 def CXX98CompatBindToTemporaryCopy :
   DiagGroup<"c++98-compat-bind-to-temporary-copy">;

 // earlier C++ versions.
 def CXX1z : DiagGroup<"c++1z-extensions">;
 
+// A warning group for warnings about using C++2a features as extensions in
+// earlier C++ versions.
+def CXX2a : DiagGroup<"c++2a-extensions">;
+
 def : DiagGroup<"c++0x-extensions", [CXX11]>;
 def : DiagGroup<"c++1y-extensions", [CXX14]>;
 

include/clang/Basic/DiagnosticSemaKinds.td

     "%0 can appear only once in a capture list">;
   def err_reference_capture_with_reference_default : Error<
     "'&' cannot precede a capture when the capture default is '&'">;
-  def err_this_capture_with_copy_default : Error<
-    "'this' cannot be explicitly captured when the capture default is '='">;
   def err_copy_capture_with_copy_default : Error<
     "'&' must precede a capture when the capture default is '='">;
   def err_capture_does_not_name_variable : Error<

      InGroup<CXXPre1zCompat>, DefaultIgnore;
   def ext_star_this_lambda_capture_cxx1z : ExtWarn<
     "capture of '*this' by copy is a C++1z extension">, InGroup<CXX1z>;
+
+  // C++2a [=, this] captures.
+  def warn_cxx1z_compat_equals_this_lambda_capture : Warning<
+    "explicit capture of 'this' with a capture default of '=' is incompatible "
+    "with C++ standards before C++2a">, InGroup<CXXPre2aCompat>, DefaultIgnore;
+  def ext_equals_this_lambda_capture_cxx2a : ExtWarn<
+    "explicit capture of 'this' with a capture default of '=' "
+    "is a C++2a extension">, InGroup<CXX2a>;
 }
 
 def err_return_in_captured_stmt : Error<

lib/Sema/SemaLambda.cpp

         continue;
       }
 
-      // C++1z [expr.prim.lambda]p8:
-      //  If a lambda-capture includes a capture-default that is =, each
-      //  simple-capture of that lambda-capture shall be of the form "&
-      //  identifier" or "* this". [ Note: The form [&,this] is redundant but
-      //  accepted for compatibility with ISO C++14. --end note ]
-      if (Intro.Default == LCD_ByCopy && C->Kind != LCK_StarThis) {
-        Diag(C->Loc, diag::err_this_capture_with_copy_default)
-            << FixItHint::CreateRemoval(
-                SourceRange(getLocForEndOfToken(PrevCaptureLoc), C->Loc));
-        continue;
-      }
+      // C++2a [expr.prim.lambda]p8:
+      //  If a lambda-capture includes a capture-default that is =,
+      //  each simple-capture of that lambda-capture shall be of the form
+      //  "&identifier", "this", or "* this". [ Note: The form [&,this] is
+      //  redundant but accepted for compatibility with ISO C++14. --end note ]
+      if (Intro.Default == LCD_ByCopy && C->Kind != LCK_StarThis)
+        Diag(C->Loc, !getLangOpts().CPlusPlus2a
+                         ? diag::ext_equals_this_lambda_capture_cxx2a
+                         : diag::warn_cxx1z_compat_equals_this_lambda_capture);
 
       // C++11 [expr.prim.lambda]p12:
       //   If this is captured by a local lambda expression, its nearest

test/CXX/expr/expr.prim/expr.prim.lambda/p8.cpp

     (void)[this, this] () {}; // expected-error {{'this' can appear only once}}
     (void)[=, foo] () {}; // expected-error {{'&' must precede a capture when}}
     (void)[=, &foo] () {};
-    (void)[=, this] () {}; // expected-error {{'this' cannot be explicitly captured}}
+    (void)[=, this] () {}; // expected-warning {{C++2a extension}}
     (void)[&, foo] () {};
     (void)[&, &foo] () {}; // expected-error {{'&' cannot precede a capture when}} 
     (void)[&, this] () {};

 void S2::f(int i) {
   (void)[&, i]{ };
   (void)[&, &i]{ }; // expected-error{{'&' cannot precede a capture when the capture default is '&'}}
-  (void)[=, this]{ }; // expected-error{{'this' cannot be explicitly captured}}
+  (void)[=, this]{ }; // expected-warning{{C++2a extension}}
   (void)[=]{ this->g(i); };
   (void)[i, i]{ }; // expected-error{{'i' can appear only once in a capture list}}
   (void)[i(0), i(1)]{ }; // expected-error{{'i' can appear only once in a capture list}}

test/FixIt/fixit-cxx0x.cpp

 
 void S2::f(int i) {
   (void)[&, &i, &i]{}; // expected-error 2{{'&' cannot precede a capture when the capture default is '&'}}
-  (void)[=, this]{ this->g(5); }; // expected-error{{'this' cannot be explicitly captured}}
   (void)[i, i]{ }; // expected-error{{'i' can appear only once in a capture list}}
   (void)[&, i, i]{ }; // expected-error{{'i' can appear only once in a capture list}}
   (void)[] mutable { }; // expected-error{{lambda requires '()' before 'mutable'}}

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

+// RUN: %clang_cc1 -std=c++2a -verify %s
+// expected-no-diagnostics
+
+// This test does two things.
+// Deleting the copy constructor ensures that an [=, this] capture doesn't copy the object.
+// Accessing a member variable from the lambda ensures that the capture actually works.
+class A {
+  A(const A &) = delete;
+  int i;
+
+  void func() {
+    auto L = [=, this]() -> int { return i; };
+    L();
+  }
+};

www/cxx_status.html

 
 <h2 id="cxx20">C++2a implementation status</h2>
 
-<p>Clang does not yet support any of the proposed features of
-<!--<p>Clang has <b>experimental</b> support for some proposed features of-->
+<p>Clang has <b>experimental</b> support for some proposed features of
 the C++ standard following C++17, provisionally named C++2a.
 Note that support for these features may change or be removed without notice,
 as the draft C++2a standard evolves.
 
-<!--<p>You can use Clang in C++2a mode with the <code>-std=c++2a</code> option.</p>-->
+<p>You can use Clang in C++2a mode with the <code>-std=c++2a</code> option.</p>
 
 <details open>
 <summary>List of features and minimum Clang version with support</summary>

     <tr>
       <td>Allow <i>lambda-capture</i> <tt>[=, this]</tt></td>
       <td><a href="http://wg21.link/p0409r2">P0409R2</a></td>
-      <td class="none" align="center">No</td>
+      <td class="svn" align="center">SVN</td>
     </tr>
     <tr>
       <td><tt>__VA_OPT__</tt> for preprocessor comma elision</td>