[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

[Faisal Vali via Phabricator via cfe-commits]

faisalv closed this revision.
faisalv added a comment.

Committed as r311224, on behalf of Hamza.

https://reviews.llvm.org/rL311224


https://reviews.llvm.org/D36572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

[Hamza Sood via Phabricator via cfe-commits]

hamzasood updated this revision to Diff 111681.
hamzasood added a comment.

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 
.


https://reviews.llvm.org/D36572

Files:
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaLambda.cpp
  test/CXX/expr/expr.prim/expr.prim.lambda/p8.cpp
  test/FixIt/fixit-cxx0x.cpp
  test/SemaCXX/cxx2a-lambda-equals-this.cpp
  www/cxx_status.html

Index: www/cxx_status.html
===
--- www/cxx_status.html
+++ www/cxx_status.html
@@ -777,13 +777,12 @@
 
 C++2a implementation status
 
-Clang does not yet support any of the proposed features of
-
+Clang has experimental 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.
 
-
+You can use Clang in C++2a mode with the -std=c++2a option.
 
 
 List of features and minimum Clang version with support
@@ -808,7 +807,7 @@
 
   Allow lambda-capture [=, this]
   http://wg21.link/p0409r2;>P0409R2
-  No
+  SVN
 
 
   __VA_OPT__ for preprocessor comma elision
Index: test/SemaCXX/cxx2a-lambda-equals-this.cpp
===
--- test/SemaCXX/cxx2a-lambda-equals-this.cpp
+++ test/SemaCXX/cxx2a-lambda-equals-this.cpp
@@ -0,0 +1,15 @@
+// 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();
+  }
+};
Index: test/FixIt/fixit-cxx0x.cpp
===
--- test/FixIt/fixit-cxx0x.cpp
+++ test/FixIt/fixit-cxx0x.cpp
@@ -54,7 +54,6 @@
 
 void S2::f(int i) {
   (void)[&, , ]{}; // 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'}}
Index: test/CXX/expr/expr.prim/expr.prim.lambda/p8.cpp
===
--- test/CXX/expr/expr.prim/expr.prim.lambda/p8.cpp
+++ test/CXX/expr/expr.prim/expr.prim.lambda/p8.cpp
@@ -8,7 +8,7 @@
 (void)[this, this] () {}; // expected-error {{'this' can appear only once}}
 (void)[=, foo] () {}; // expected-error {{'&' must precede a capture when}}
 (void)[=, ] () {};
-(void)[=, this] () {}; // expected-error {{'this' cannot be explicitly captured}}
+(void)[=, this] () {}; // expected-warning {{C++2a extension}}
 (void)[&, foo] () {};
 (void)[&, ] () {}; // expected-error {{'&' cannot precede a capture when}} 
 (void)[&, this] () {};
@@ -23,7 +23,7 @@
 void S2::f(int i) {
   (void)[&, i]{ };
   (void)[&, ]{ }; // 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}}
Index: lib/Sema/SemaLambda.cpp
===
--- lib/Sema/SemaLambda.cpp
+++ lib/Sema/SemaLambda.cpp
@@ -948,17 +948,15 @@
 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 

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: test/FixIt/fixit-cxx0x.cpp:57
   (void)[&, , ]{}; // 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}}

hamzasood wrote:
> rjmccall wrote:
> > rjmccall wrote:
> > > hamzasood wrote:
> > > > rjmccall wrote:
> > > > > hamzasood wrote:
> > > > > > rjmccall wrote:
> > > > > > > hamzasood wrote:
> > > > > > > > rjmccall wrote:
> > > > > > > > > Shouldn't you only be accepting this in C++2a mode?
> > > > > > > > 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]).
> > > > > > > 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?
> > > > > > The extension warning for this 
> > > > > > (ext_equals_this_lambda_capture_cxx2a) is on by default.
> > > > > Why did the diagnostic disappear from this file, then?
> > > > 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)
> > > 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.
> > 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?
> Ah okay, now I know what you mean. The line you’re talking about has actually 
> been removed completely from the fixit test.
Oh, of course, I should've realized that right away.  Sorry for the run-around.

In that case, this all LGTM.


https://reviews.llvm.org/D36572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

[Hamza Sood via Phabricator via cfe-commits]

hamzasood added inline comments.



Comment at: test/FixIt/fixit-cxx0x.cpp:57
   (void)[&, , ]{}; // 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}}

rjmccall wrote:
> rjmccall wrote:
> > hamzasood wrote:
> > > rjmccall wrote:
> > > > hamzasood wrote:
> > > > > rjmccall wrote:
> > > > > > hamzasood wrote:
> > > > > > > rjmccall wrote:
> > > > > > > > Shouldn't you only be accepting this in C++2a mode?
> > > > > > > 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]).
> > > > > > 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?
> > > > > The extension warning for this (ext_equals_this_lambda_capture_cxx2a) 
> > > > > is on by default.
> > > > Why did the diagnostic disappear from this file, then?
> > > 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)
> > 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.
> 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?
Ah okay, now I know what you mean. The line you’re talking about has actually 
been removed completely from the fixit test.


https://reviews.llvm.org/D36572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/Sema/SemaLambda.cpp:959
+ ? diag::ext_equals_this_lambda_capture_cxx2a
+ : diag::warn_cxx1z_compat_equals_this_lambda_capture);
 

hamzasood wrote:
> faisalv wrote:
> > Shouldn't we try and hit the 'continue' (that u deleted) if warnings (and 
> > extension warnings) are turned into errors? 
> That's an interesting scenario which admittedly I hadn't considered.
> 
> I based this implementation on the [[ 
> https://github.com/llvm-mirror/clang/blob/7602b13a8e8b5656afd6327d112b76b39f836e5b/lib/Sema/SemaLambda.cpp#L935
>  | '*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.
Yeah, I doubt there's a single place in the compiler where we stop processing 
code when warnings are turned into errors.



Comment at: test/FixIt/fixit-cxx0x.cpp:57
   (void)[&, , ]{}; // 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}}

rjmccall wrote:
> hamzasood wrote:
> > rjmccall wrote:
> > > hamzasood wrote:
> > > > rjmccall wrote:
> > > > > hamzasood wrote:
> > > > > > rjmccall wrote:
> > > > > > > Shouldn't you only be accepting this in C++2a mode?
> > > > > > 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]).
> > > > > 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?
> > > > The extension warning for this (ext_equals_this_lambda_capture_cxx2a) 
> > > > is on by default.
> > > Why did the diagnostic disappear from this file, then?
> > 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)
> 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.
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?


https://reviews.llvm.org/D36572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

[Faisal Vali via Phabricator via cfe-commits]

faisalv accepted this revision.
faisalv added a comment.
This revision is now accepted and ready to land.

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!


https://reviews.llvm.org/D36572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

[Hamza Sood via Phabricator via cfe-commits]

hamzasood added inline comments.



Comment at: lib/Sema/SemaLambda.cpp:959
+ ? diag::ext_equals_this_lambda_capture_cxx2a
+ : diag::warn_cxx1z_compat_equals_this_lambda_capture);
 

faisalv wrote:
> Shouldn't we try and hit the 'continue' (that u deleted) if warnings (and 
> extension warnings) are turned into errors? 
That's an interesting scenario which admittedly I hadn't considered.

I based this implementation on the [[ 
https://github.com/llvm-mirror/clang/blob/7602b13a8e8b5656afd6327d112b76b39f836e5b/lib/Sema/SemaLambda.cpp#L935
 | '*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.


https://reviews.llvm.org/D36572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

[Faisal Vali via Phabricator via cfe-commits]

faisalv added inline comments.



Comment at: lib/Sema/SemaLambda.cpp:959
+ ? diag::ext_equals_this_lambda_capture_cxx2a
+ : diag::warn_cxx1z_compat_equals_this_lambda_capture);
 

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



Comment at: test/SemaCXX/cxx2a-lambda-equals-this.cpp:6
+// 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 {

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


https://reviews.llvm.org/D36572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

[Hamza Sood via Phabricator via cfe-commits]

hamzasood updated this revision to Diff 110776.
hamzasood added a comment.

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


https://reviews.llvm.org/D36572

Files:
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaLambda.cpp
  test/CXX/expr/expr.prim/expr.prim.lambda/p8.cpp
  test/FixIt/fixit-cxx0x.cpp
  test/SemaCXX/cxx2a-lambda-equals-this.cpp
  www/cxx_status.html

Index: www/cxx_status.html
===
--- www/cxx_status.html
+++ www/cxx_status.html
@@ -777,13 +777,12 @@
 
 C++2a implementation status
 
-Clang does not yet support any of the proposed features of
-
+Clang has experimental 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.
 
-
+You can use Clang in C++2a mode with the -std=c++2a option.
 
 
 List of features and minimum Clang version with support
@@ -808,7 +807,7 @@
 
   Allow lambda-capture [=, this]
   http://wg21.link/p0409r2;>P0409R2
-  No
+  SVN
 
 
   __VA_OPT__ for preprocessor comma elision
Index: test/SemaCXX/cxx2a-lambda-equals-this.cpp
===
--- test/SemaCXX/cxx2a-lambda-equals-this.cpp
+++ test/SemaCXX/cxx2a-lambda-equals-this.cpp
@@ -0,0 +1,15 @@
+// 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();
+  }
+};
Index: test/FixIt/fixit-cxx0x.cpp
===
--- test/FixIt/fixit-cxx0x.cpp
+++ test/FixIt/fixit-cxx0x.cpp
@@ -54,7 +54,6 @@
 
 void S2::f(int i) {
   (void)[&, , ]{}; // 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'}}
Index: test/CXX/expr/expr.prim/expr.prim.lambda/p8.cpp
===
--- test/CXX/expr/expr.prim/expr.prim.lambda/p8.cpp
+++ test/CXX/expr/expr.prim/expr.prim.lambda/p8.cpp
@@ -8,7 +8,7 @@
 (void)[this, this] () {}; // expected-error {{'this' can appear only once}}
 (void)[=, foo] () {}; // expected-error {{'&' must precede a capture when}}
 (void)[=, ] () {};
-(void)[=, this] () {}; // expected-error {{'this' cannot be explicitly captured}}
+(void)[=, this] () {}; // expected-warning {{C++2a extension}}
 (void)[&, foo] () {};
 (void)[&, ] () {}; // expected-error {{'&' cannot precede a capture when}} 
 (void)[&, this] () {};
@@ -23,7 +23,7 @@
 void S2::f(int i) {
   (void)[&, i]{ };
   (void)[&, ]{ }; // 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}}
Index: lib/Sema/SemaLambda.cpp
===
--- lib/Sema/SemaLambda.cpp
+++ lib/Sema/SemaLambda.cpp
@@ -948,17 +948,15 @@
 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

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: test/FixIt/fixit-cxx0x.cpp:57
   (void)[&, , ]{}; // 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}}

hamzasood wrote:
> rjmccall wrote:
> > hamzasood wrote:
> > > rjmccall wrote:
> > > > hamzasood wrote:
> > > > > rjmccall wrote:
> > > > > > Shouldn't you only be accepting this in C++2a mode?
> > > > > 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]).
> > > > 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?
> > > The extension warning for this (ext_equals_this_lambda_capture_cxx2a) is 
> > > on by default.
> > Why did the diagnostic disappear from this file, then?
> 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)
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.


https://reviews.llvm.org/D36572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

[Hamza Sood via Phabricator via cfe-commits]

hamzasood added inline comments.



Comment at: test/FixIt/fixit-cxx0x.cpp:57
   (void)[&, , ]{}; // 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}}

rjmccall wrote:
> hamzasood wrote:
> > rjmccall wrote:
> > > hamzasood wrote:
> > > > rjmccall wrote:
> > > > > Shouldn't you only be accepting this in C++2a mode?
> > > > 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]).
> > > 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?
> > The extension warning for this (ext_equals_this_lambda_capture_cxx2a) is on 
> > by default.
> Why did the diagnostic disappear from this file, then?
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)


https://reviews.llvm.org/D36572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: test/FixIt/fixit-cxx0x.cpp:57
   (void)[&, , ]{}; // 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}}

hamzasood wrote:
> rjmccall wrote:
> > hamzasood wrote:
> > > rjmccall wrote:
> > > > Shouldn't you only be accepting this in C++2a mode?
> > > 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]).
> > 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?
> The extension warning for this (ext_equals_this_lambda_capture_cxx2a) is on 
> by default.
Why did the diagnostic disappear from this file, then?


https://reviews.llvm.org/D36572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

[Hamza Sood via Phabricator via cfe-commits]

hamzasood added inline comments.



Comment at: test/FixIt/fixit-cxx0x.cpp:57
   (void)[&, , ]{}; // 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}}

rjmccall wrote:
> hamzasood wrote:
> > rjmccall wrote:
> > > Shouldn't you only be accepting this in C++2a mode?
> > 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]).
> 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?
The extension warning for this (ext_equals_this_lambda_capture_cxx2a) is on by 
default.


https://reviews.llvm.org/D36572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: test/FixIt/fixit-cxx0x.cpp:57
   (void)[&, , ]{}; // 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}}

hamzasood wrote:
> rjmccall wrote:
> > Shouldn't you only be accepting this in C++2a mode?
> 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]).
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?


https://reviews.llvm.org/D36572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

[Hamza Sood via Phabricator via cfe-commits]

hamzasood added inline comments.



Comment at: test/FixIt/fixit-cxx0x.cpp:57
   (void)[&, , ]{}; // 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}}

rjmccall wrote:
> Shouldn't you only be accepting this in C++2a mode?
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]).


https://reviews.llvm.org/D36572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: test/FixIt/fixit-cxx0x.cpp:57
   (void)[&, , ]{}; // 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}}

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


https://reviews.llvm.org/D36572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

[Hamza Sood via Phabricator via cfe-commits]

hamzasood created this revision.

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.


https://reviews.llvm.org/D36572

Files:
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaLambda.cpp
  test/CXX/expr/expr.prim/expr.prim.lambda/p8.cpp
  test/FixIt/fixit-cxx0x.cpp
  test/SemaCXX/cxx2a-lambda-equals-this.cpp
  www/cxx_status.html

Index: www/cxx_status.html
===
--- www/cxx_status.html
+++ www/cxx_status.html
@@ -777,13 +777,12 @@
 
 C++2a implementation status
 
-Clang does not yet support any of the proposed features of
-
+Clang has experimental 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.
 
-
+You can use Clang in C++2a mode with the -std=c++2a option.
 
 
 List of features and minimum Clang version with support
@@ -808,7 +807,7 @@
 
   Allow lambda-capture [=, this]
   http://wg21.link/p0409r2;>P0409R2
-  No
+  SVN
 
 
   __VA_OPT__ for preprocessor comma elision
Index: test/SemaCXX/cxx2a-lambda-equals-this.cpp
===
--- test/SemaCXX/cxx2a-lambda-equals-this.cpp
+++ test/SemaCXX/cxx2a-lambda-equals-this.cpp
@@ -0,0 +1,15 @@
+// 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();
+  }
+};
Index: test/FixIt/fixit-cxx0x.cpp
===
--- test/FixIt/fixit-cxx0x.cpp
+++ test/FixIt/fixit-cxx0x.cpp
@@ -54,7 +54,6 @@
 
 void S2::f(int i) {
   (void)[&, , ]{}; // 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'}}
Index: test/CXX/expr/expr.prim/expr.prim.lambda/p8.cpp
===
--- test/CXX/expr/expr.prim/expr.prim.lambda/p8.cpp
+++ test/CXX/expr/expr.prim/expr.prim.lambda/p8.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++11 %s -verify -Wno-c++1y-extensions
+// RUN: %clang_cc1 -std=c++11 %s -verify -Wno-c++1y-extensions -Wno-c++2a-extensions
 
 class X0 {
   void explicit_capture() {
@@ -8,7 +8,7 @@
 (void)[this, this] () {}; // expected-error {{'this' can appear only once}}
 (void)[=, foo] () {}; // expected-error {{'&' must precede a capture when}}
 (void)[=, ] () {};
-(void)[=, this] () {}; // expected-error {{'this' cannot be explicitly captured}}
+(void)[=, this] () {};
 (void)[&, foo] () {};
 (void)[&, ] () {}; // expected-error {{'&' cannot precede a capture when}} 
 (void)[&, this] () {};
@@ -23,7 +23,7 @@
 void S2::f(int i) {
   (void)[&, i]{ };
   (void)[&, ]{ }; // expected-error{{'&' cannot precede a capture when the capture default is '&'}}
-  (void)[=, this]{ }; // expected-error{{'this' cannot be explicitly captured}}
+  (void)[=, this]{ };
   (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}}
Index: lib/Sema/SemaLambda.cpp
===
--- lib/Sema/SemaLambda.cpp
+++ lib/Sema/SemaLambda.cpp
@@ -948,17 +948,15 @@
 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