[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons added inline comments.



Comment at: lib/Sema/SemaLambda.cpp:1567
+if (!CurHasPreviousCapture && !IsLast) {
+  // If there are no captures preceding this capture, remove the
+  // following comma.

In clang-tidy checks, removal of commas is handled automatically by 
`format::cleanupAroundReplacements()`.
Is that not the case in clang?


Repository:
  rC Clang

https://reviews.llvm.org/D48845



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

[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons added a comment.

In https://reviews.llvm.org/D48845#1158103, @alexshap wrote:

> I'm kind of interested in this fixit, but one thought which i have - probably 
> it should be more conservative (i.e. fix captures by reference, integral 
> types, etc) (since the code might rely on side-effects of 
> copy-ctors/move-ctors or extend the lifetime of an object), but fixing only 
> simple cases still seems to be useful imo.


The warning is already conservative.


Repository:
  rC Clang

https://reviews.llvm.org/D48845



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

[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Alexander Shaposhnikov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC337148: [Sema] Add fixit for unused lambda captures 
(authored by alexshap, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D48845?vs=155616=155624#toc

Repository:
  rC Clang

https://reviews.llvm.org/D48845

Files:
  include/clang/Sema/DeclSpec.h
  include/clang/Sema/ScopeInfo.h
  include/clang/Sema/Sema.h
  lib/Parse/ParseExprCXX.cpp
  lib/Sema/SemaLambda.cpp
  test/FixIt/fixit-unused-lambda-capture.cpp

Index: lib/Sema/SemaLambda.cpp
===
--- lib/Sema/SemaLambda.cpp
+++ lib/Sema/SemaLambda.cpp
@@ -993,6 +993,8 @@
   CheckCXXThisCapture(C->Loc, /*Explicit=*/true, /*BuildAndDiagnose*/ true,
   /*FunctionScopeIndexToStopAtPtr*/ nullptr,
   C->Kind == LCK_StarThis);
+  if (!LSI->Captures.empty())
+LSI->ExplicitCaptureRanges[LSI->Captures.size() - 1] = C->ExplicitRange;
   continue;
 }
 
@@ -1139,6 +1141,8 @@
TryCapture_ExplicitByVal;
   tryCaptureVariable(Var, C->Loc, Kind, EllipsisLoc);
 }
+if (!LSI->Captures.empty())
+  LSI->ExplicitCaptureRanges[LSI->Captures.size() - 1] = C->ExplicitRange;
   }
   finishLambdaExplicitCaptures(LSI);
 
@@ -1478,19 +1482,22 @@
   return false;
 }
 
-void Sema::DiagnoseUnusedLambdaCapture(const Capture ) {
+bool Sema::DiagnoseUnusedLambdaCapture(SourceRange CaptureRange,
+   const Capture ) {
   if (CaptureHasSideEffects(From))
-return;
+return false;
 
   if (From.isVLATypeCapture())
-return;
+return false;
 
   auto diag = Diag(From.getLocation(), diag::warn_unused_lambda_capture);
   if (From.isThisCapture())
 diag << "'this'";
   else
 diag << From.getVariable();
   diag << From.isNonODRUsed();
+  diag << FixItHint::CreateRemoval(CaptureRange);
+  return true;
 }
 
 ExprResult Sema::BuildLambdaExpr(SourceLocation StartLoc, SourceLocation EndLoc,
@@ -1532,18 +1539,49 @@
 
 // Translate captures.
 auto CurField = Class->field_begin();
+// True if the current capture has a used capture or default before it.
+bool CurHasPreviousCapture = CaptureDefault != LCD_None;
+SourceLocation PrevCaptureLoc = CurHasPreviousCapture ?
+CaptureDefaultLoc : IntroducerRange.getBegin();
+
 for (unsigned I = 0, N = LSI->Captures.size(); I != N; ++I, ++CurField) {
   const Capture  = LSI->Captures[I];
+
   assert(!From.isBlockCapture() && "Cannot capture __block variables");
   bool IsImplicit = I >= LSI->NumExplicitCaptures;
 
+  // Use source ranges of explicit captures for fixits where available.
+  SourceRange CaptureRange = LSI->ExplicitCaptureRanges[I];
+
   // Warn about unused explicit captures.
+  bool IsCaptureUsed = true;
   if (!CurContext->isDependentContext() && !IsImplicit && !From.isODRUsed()) {
 // Initialized captures that are non-ODR used may not be eliminated.
 bool NonODRUsedInitCapture =
 IsGenericLambda && From.isNonODRUsed() && From.getInitExpr();
-if (!NonODRUsedInitCapture)
-  DiagnoseUnusedLambdaCapture(From);
+if (!NonODRUsedInitCapture) {
+  bool IsLast = (I + 1) == LSI->NumExplicitCaptures;
+	SourceRange FixItRange;
+  if (CaptureRange.isValid()) {
+if (!CurHasPreviousCapture && !IsLast) {
+  // If there are no captures preceding this capture, remove the
+  // following comma.
+  FixItRange = SourceRange(CaptureRange.getBegin(),
+   getLocForEndOfToken(CaptureRange.getEnd()));
+} else {
+  // Otherwise, remove the comma since the last used capture.
+  FixItRange = SourceRange(getLocForEndOfToken(PrevCaptureLoc),
+   CaptureRange.getEnd());
+}
+  }
+
+  IsCaptureUsed = !DiagnoseUnusedLambdaCapture(FixItRange, From);
+}
+  }
+
+  if (CaptureRange.isValid()) {
+CurHasPreviousCapture |= IsCaptureUsed;
+PrevCaptureLoc = CaptureRange.getEnd();
   }
 
   // Handle 'this' capture.
Index: lib/Parse/ParseExprCXX.cpp
===
--- lib/Parse/ParseExprCXX.cpp
+++ lib/Parse/ParseExprCXX.cpp
@@ -808,6 +808,7 @@
 IdentifierInfo *Id = nullptr;
 SourceLocation EllipsisLoc;
 ExprResult Init;
+SourceLocation LocStart = Tok.getLocation();
 
 if (Tok.is(tok::star)) {
   Loc = ConsumeToken(); 
@@ -981,8 +982,11 @@
   Loc, Kind == LCK_ByRef, Id, InitKind, InitExpr);
   Init = InitExpr;
 }
+
+SourceLocation LocEnd = PrevTokLocation;
+
 Intro.addCapture(Kind, Loc, Id, EllipsisLoc, InitKind, Init,
- InitCaptureType);
+ 

[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Andrew Comminos via Phabricator via cfe-commits]

acomminos updated this revision to Diff 155616.
acomminos added a comment.

Remove `const` qualifier for SourceRange.


Repository:
  rC Clang

https://reviews.llvm.org/D48845

Files:
  include/clang/Sema/DeclSpec.h
  include/clang/Sema/ScopeInfo.h
  include/clang/Sema/Sema.h
  lib/Parse/ParseExprCXX.cpp
  lib/Sema/SemaLambda.cpp
  test/FixIt/fixit-unused-lambda-capture.cpp

Index: test/FixIt/fixit-unused-lambda-capture.cpp
===
--- /dev/null
+++ test/FixIt/fixit-unused-lambda-capture.cpp
@@ -0,0 +1,94 @@
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wunused-lambda-capture -Wno-unused-value -std=c++1z -fixit %t
+// RUN: grep -v CHECK %t | FileCheck %s
+
+void test() {
+  int i = 0;
+  int j = 0;
+  int k = 0;
+  int c = 10;
+  int a[c];
+
+  [i,j] { return i; };
+  // CHECK: [i] { return i; };
+  [i,j] { return j; };
+  // CHECK: [j] { return j; };
+  [,j] { return j; };
+  // CHECK: [j] { return j; };
+  [j,] { return j; };
+  // CHECK: [j] { return j; };
+  [i,j,k] {};
+  // CHECK: [] {};
+  [i,j,k] { return i + j; };
+  // CHECK: [i,j] { return i + j; };
+  [i,j,k] { return j + k; };
+  // CHECK: [j,k] { return j + k; };
+  [i,j,k] { return i + k; };
+  // CHECK: [i,k] { return i + k; };
+  [i,j,k] { return i + j + k; };
+  // CHECK: [i,j,k] { return i + j + k; };
+  [&,i] { return k; };
+  // CHECK: [&] { return k; };
+  [=,] { return k; };
+  // CHECK: [=] { return k; };
+  [=,,] { return j; };
+  // CHECK: [=,] { return j; };
+  [=,,] { return i; };
+  // CHECK: [=,] { return i; };
+  [z = i] {};
+  // CHECK: [] {};
+  [i,z = i] { return z; };
+  // CHECK: [z = i] { return z; };
+  [z = i,i] { return z; };
+  // CHECK: [z = i] { return z; };
+  [] {};
+  // CHECK: [] {};
+  [i,] { return i; };
+  // CHECK: [i] { return i; };
+  [,i] { return i; };
+  // CHECK: [i] { return i; };
+
+  #define I_MACRO() i
+  #define I_REF_MACRO() 
+  [I_MACRO()] {};
+  // CHECK: [] {};
+  [I_MACRO(),j] { return j; };
+  // CHECK: [j] { return j; };
+  [j,I_MACRO()] { return j; };
+  // CHECK: [j] { return j; };
+  [I_REF_MACRO(),j] { return j; };
+  // CHECK: [j] { return j; };
+  [j,I_REF_MACRO()] { return j; };
+  // CHECK: [j] { return j; };
+
+  int n = 0;
+  [z = (n = i),j] {};
+  // CHECK: [z = (n = i)] {};
+  [j,z = (n = i)] {};
+  // CHECK: [z = (n = i)] {};
+}
+
+class ThisTest {
+  void test() {
+int i = 0;
+
+[this] {};
+// CHECK: [] {};
+[i,this] { return i; };
+// CHECK: [i] { return i; };
+[this,i] { return i; };
+// CHECK: [i] { return i; };
+[*this] {};
+// CHECK: [] {};
+[*this,i] { return i; };
+// CHECK: [i] { return i; };
+[i,*this] { return i; };
+// CHECK: [i] { return i; };
+[*this] { return this; };
+// CHECK: [*this] { return this; };
+[*this,i] { return this; };
+// CHECK: [*this] { return this; };
+[i,*this] { return this; };
+// CHECK: [*this] { return this; };
+  }
+};
Index: lib/Sema/SemaLambda.cpp
===
--- lib/Sema/SemaLambda.cpp
+++ lib/Sema/SemaLambda.cpp
@@ -993,6 +993,7 @@
   CheckCXXThisCapture(C->Loc, /*Explicit=*/true, /*BuildAndDiagnose*/ true,
   /*FunctionScopeIndexToStopAtPtr*/ nullptr,
   C->Kind == LCK_StarThis);
+  LSI->ExplicitCaptureRanges[LSI->Captures.size() - 1] = C->ExplicitRange;
   continue;
 }
 
@@ -1139,6 +1140,7 @@
TryCapture_ExplicitByVal;
   tryCaptureVariable(Var, C->Loc, Kind, EllipsisLoc);
 }
+LSI->ExplicitCaptureRanges[LSI->Captures.size() - 1] = C->ExplicitRange;
   }
   finishLambdaExplicitCaptures(LSI);
 
@@ -1478,19 +1480,22 @@
   return false;
 }
 
-void Sema::DiagnoseUnusedLambdaCapture(const Capture ) {
+bool Sema::DiagnoseUnusedLambdaCapture(SourceRange CaptureRange,
+   const Capture ) {
   if (CaptureHasSideEffects(From))
-return;
+return false;
 
   if (From.isVLATypeCapture())
-return;
+return false;
 
   auto diag = Diag(From.getLocation(), diag::warn_unused_lambda_capture);
   if (From.isThisCapture())
 diag << "'this'";
   else
 diag << From.getVariable();
   diag << From.isNonODRUsed();
+  diag << FixItHint::CreateRemoval(CaptureRange);
+  return true;
 }
 
 ExprResult Sema::BuildLambdaExpr(SourceLocation StartLoc, SourceLocation EndLoc,
@@ -1532,18 +1537,49 @@
 
 // Translate captures.
 auto CurField = Class->field_begin();
+// True if the current capture has a used capture or default before it.
+bool CurHasPreviousCapture = CaptureDefault != LCD_None;
+SourceLocation PrevCaptureLoc = CurHasPreviousCapture ?
+CaptureDefaultLoc : IntroducerRange.getBegin();
+
 for (unsigned I = 0, N = LSI->Captures.size(); I != N; ++I, ++CurField) {
   const Capture  = LSI->Captures[I];
+
   assert(!From.isBlockCapture() 

[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

Aside from a small nit in the comments, LGTM.




Comment at: include/clang/Sema/Sema.h:5608
+  /// diagnostic is emitted.
+  bool DiagnoseUnusedLambdaCapture(const SourceRange CaptureRange,
+   const sema::Capture );

No need to mark the SourceRange const, unless you intended to pass it by 
reference.


Repository:
  rC Clang

https://reviews.llvm.org/D48845



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

[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Alexander Shaposhnikov via Phabricator via cfe-commits]

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

to me LG


Repository:
  rC Clang

https://reviews.llvm.org/D48845



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

[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Andrew Comminos via Phabricator via cfe-commits]

acomminos updated this revision to Diff 155472.
acomminos marked an inline comment as done.
acomminos added a comment.

Use source ranges instead of a pair of source locations for explicit lambda 
captures.


Repository:
  rC Clang

https://reviews.llvm.org/D48845

Files:
  include/clang/Sema/DeclSpec.h
  include/clang/Sema/ScopeInfo.h
  include/clang/Sema/Sema.h
  lib/Parse/ParseExprCXX.cpp
  lib/Sema/SemaLambda.cpp
  test/FixIt/fixit-unused-lambda-capture.cpp

Index: test/FixIt/fixit-unused-lambda-capture.cpp
===
--- /dev/null
+++ test/FixIt/fixit-unused-lambda-capture.cpp
@@ -0,0 +1,94 @@
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wunused-lambda-capture -Wno-unused-value -std=c++1z -fixit %t
+// RUN: grep -v CHECK %t | FileCheck %s
+
+void test() {
+  int i = 0;
+  int j = 0;
+  int k = 0;
+  int c = 10;
+  int a[c];
+
+  [i,j] { return i; };
+  // CHECK: [i] { return i; };
+  [i,j] { return j; };
+  // CHECK: [j] { return j; };
+  [,j] { return j; };
+  // CHECK: [j] { return j; };
+  [j,] { return j; };
+  // CHECK: [j] { return j; };
+  [i,j,k] {};
+  // CHECK: [] {};
+  [i,j,k] { return i + j; };
+  // CHECK: [i,j] { return i + j; };
+  [i,j,k] { return j + k; };
+  // CHECK: [j,k] { return j + k; };
+  [i,j,k] { return i + k; };
+  // CHECK: [i,k] { return i + k; };
+  [i,j,k] { return i + j + k; };
+  // CHECK: [i,j,k] { return i + j + k; };
+  [&,i] { return k; };
+  // CHECK: [&] { return k; };
+  [=,] { return k; };
+  // CHECK: [=] { return k; };
+  [=,,] { return j; };
+  // CHECK: [=,] { return j; };
+  [=,,] { return i; };
+  // CHECK: [=,] { return i; };
+  [z = i] {};
+  // CHECK: [] {};
+  [i,z = i] { return z; };
+  // CHECK: [z = i] { return z; };
+  [z = i,i] { return z; };
+  // CHECK: [z = i] { return z; };
+  [] {};
+  // CHECK: [] {};
+  [i,] { return i; };
+  // CHECK: [i] { return i; };
+  [,i] { return i; };
+  // CHECK: [i] { return i; };
+
+  #define I_MACRO() i
+  #define I_REF_MACRO() 
+  [I_MACRO()] {};
+  // CHECK: [] {};
+  [I_MACRO(),j] { return j; };
+  // CHECK: [j] { return j; };
+  [j,I_MACRO()] { return j; };
+  // CHECK: [j] { return j; };
+  [I_REF_MACRO(),j] { return j; };
+  // CHECK: [j] { return j; };
+  [j,I_REF_MACRO()] { return j; };
+  // CHECK: [j] { return j; };
+
+  int n = 0;
+  [z = (n = i),j] {};
+  // CHECK: [z = (n = i)] {};
+  [j,z = (n = i)] {};
+  // CHECK: [z = (n = i)] {};
+}
+
+class ThisTest {
+  void test() {
+int i = 0;
+
+[this] {};
+// CHECK: [] {};
+[i,this] { return i; };
+// CHECK: [i] { return i; };
+[this,i] { return i; };
+// CHECK: [i] { return i; };
+[*this] {};
+// CHECK: [] {};
+[*this,i] { return i; };
+// CHECK: [i] { return i; };
+[i,*this] { return i; };
+// CHECK: [i] { return i; };
+[*this] { return this; };
+// CHECK: [*this] { return this; };
+[*this,i] { return this; };
+// CHECK: [*this] { return this; };
+[i,*this] { return this; };
+// CHECK: [*this] { return this; };
+  }
+};
Index: lib/Sema/SemaLambda.cpp
===
--- lib/Sema/SemaLambda.cpp
+++ lib/Sema/SemaLambda.cpp
@@ -993,6 +993,7 @@
   CheckCXXThisCapture(C->Loc, /*Explicit=*/true, /*BuildAndDiagnose*/ true,
   /*FunctionScopeIndexToStopAtPtr*/ nullptr,
   C->Kind == LCK_StarThis);
+  LSI->ExplicitCaptureRanges[LSI->Captures.size() - 1] = C->ExplicitRange;
   continue;
 }
 
@@ -1139,6 +1140,7 @@
TryCapture_ExplicitByVal;
   tryCaptureVariable(Var, C->Loc, Kind, EllipsisLoc);
 }
+LSI->ExplicitCaptureRanges[LSI->Captures.size() - 1] = C->ExplicitRange;
   }
   finishLambdaExplicitCaptures(LSI);
 
@@ -1478,19 +1480,22 @@
   return false;
 }
 
-void Sema::DiagnoseUnusedLambdaCapture(const Capture ) {
+bool Sema::DiagnoseUnusedLambdaCapture(const SourceRange CaptureRange,
+   const Capture ) {
   if (CaptureHasSideEffects(From))
-return;
+return false;
 
   if (From.isVLATypeCapture())
-return;
+return false;
 
   auto diag = Diag(From.getLocation(), diag::warn_unused_lambda_capture);
   if (From.isThisCapture())
 diag << "'this'";
   else
 diag << From.getVariable();
   diag << From.isNonODRUsed();
+  diag << FixItHint::CreateRemoval(CaptureRange);
+  return true;
 }
 
 ExprResult Sema::BuildLambdaExpr(SourceLocation StartLoc, SourceLocation EndLoc,
@@ -1532,18 +1537,49 @@
 
 // Translate captures.
 auto CurField = Class->field_begin();
+// True if the current capture has a used capture or default before it.
+bool CurHasPreviousCapture = CaptureDefault != LCD_None;
+SourceLocation PrevCaptureLoc = CurHasPreviousCapture ?
+CaptureDefaultLoc : IntroducerRange.getBegin();
+
 for (unsigned I = 0, N = LSI->Captures.size(); I != N; ++I, 

[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Andrew Comminos via Phabricator via cfe-commits]

acomminos marked an inline comment as done.
acomminos added inline comments.



Comment at: include/clang/Sema/DeclSpec.h:2552-2553
 ParsedType InitCaptureType;
+SourceLocation LocStart;
+SourceLocation LocEnd;
+

aaron.ballman wrote:
> aaron.ballman wrote:
> > How does `LocStart` relate to the existing source location `Loc`? I think 
> > this should have a more descriptive name of what location is involved.
> Now that I think about this more, I wonder if this is better expressed as 
> `SourceRange CaptureRange;` given that there's always a start and end and 
> they should never be equal?
Sure, makes sense to me. Many other classes with attached range data appear to 
use explicit start/end locations, but in this case I think it's fine.


Repository:
  rC Clang

https://reviews.llvm.org/D48845



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

[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/Sema/DeclSpec.h:2552-2553
 ParsedType InitCaptureType;
+SourceLocation LocStart;
+SourceLocation LocEnd;
+

aaron.ballman wrote:
> How does `LocStart` relate to the existing source location `Loc`? I think 
> this should have a more descriptive name of what location is involved.
Now that I think about this more, I wonder if this is better expressed as 
`SourceRange CaptureRange;` given that there's always a start and end and they 
should never be equal?


Repository:
  rC Clang

https://reviews.llvm.org/D48845



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

[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Andrew Comminos via Phabricator via cfe-commits]

acomminos updated this revision to Diff 155446.
acomminos marked 2 inline comments as done.
acomminos added a comment.

Add test for stateful initializer expressions not being removed, propagate 
whether or not a diagnostic actually get emitted.


Repository:
  rC Clang

https://reviews.llvm.org/D48845

Files:
  include/clang/Sema/DeclSpec.h
  include/clang/Sema/ScopeInfo.h
  include/clang/Sema/Sema.h
  lib/Parse/ParseExprCXX.cpp
  lib/Sema/SemaLambda.cpp
  test/FixIt/fixit-unused-lambda-capture.cpp

Index: test/FixIt/fixit-unused-lambda-capture.cpp
===
--- /dev/null
+++ test/FixIt/fixit-unused-lambda-capture.cpp
@@ -0,0 +1,94 @@
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wunused-lambda-capture -Wno-unused-value -std=c++1z -fixit %t
+// RUN: grep -v CHECK %t | FileCheck %s
+
+void test() {
+  int i = 0;
+  int j = 0;
+  int k = 0;
+  int c = 10;
+  int a[c];
+
+  [i,j] { return i; };
+  // CHECK: [i] { return i; };
+  [i,j] { return j; };
+  // CHECK: [j] { return j; };
+  [,j] { return j; };
+  // CHECK: [j] { return j; };
+  [j,] { return j; };
+  // CHECK: [j] { return j; };
+  [i,j,k] {};
+  // CHECK: [] {};
+  [i,j,k] { return i + j; };
+  // CHECK: [i,j] { return i + j; };
+  [i,j,k] { return j + k; };
+  // CHECK: [j,k] { return j + k; };
+  [i,j,k] { return i + k; };
+  // CHECK: [i,k] { return i + k; };
+  [i,j,k] { return i + j + k; };
+  // CHECK: [i,j,k] { return i + j + k; };
+  [&,i] { return k; };
+  // CHECK: [&] { return k; };
+  [=,] { return k; };
+  // CHECK: [=] { return k; };
+  [=,,] { return j; };
+  // CHECK: [=,] { return j; };
+  [=,,] { return i; };
+  // CHECK: [=,] { return i; };
+  [z = i] {};
+  // CHECK: [] {};
+  [i,z = i] { return z; };
+  // CHECK: [z = i] { return z; };
+  [z = i,i] { return z; };
+  // CHECK: [z = i] { return z; };
+  [] {};
+  // CHECK: [] {};
+  [i,] { return i; };
+  // CHECK: [i] { return i; };
+  [,i] { return i; };
+  // CHECK: [i] { return i; };
+
+  #define I_MACRO() i
+  #define I_REF_MACRO() 
+  [I_MACRO()] {};
+  // CHECK: [] {};
+  [I_MACRO(),j] { return j; };
+  // CHECK: [j] { return j; };
+  [j,I_MACRO()] { return j; };
+  // CHECK: [j] { return j; };
+  [I_REF_MACRO(),j] { return j; };
+  // CHECK: [j] { return j; };
+  [j,I_REF_MACRO()] { return j; };
+  // CHECK: [j] { return j; };
+
+  int n = 0;
+  [z = (n = i),j] {};
+  // CHECK: [z = (n = i)] {};
+  [j,z = (n = i)] {};
+  // CHECK: [z = (n = i)] {};
+}
+
+class ThisTest {
+  void test() {
+int i = 0;
+
+[this] {};
+// CHECK: [] {};
+[i,this] { return i; };
+// CHECK: [i] { return i; };
+[this,i] { return i; };
+// CHECK: [i] { return i; };
+[*this] {};
+// CHECK: [] {};
+[*this,i] { return i; };
+// CHECK: [i] { return i; };
+[i,*this] { return i; };
+// CHECK: [i] { return i; };
+[*this] { return this; };
+// CHECK: [*this] { return this; };
+[*this,i] { return this; };
+// CHECK: [*this] { return this; };
+[i,*this] { return this; };
+// CHECK: [*this] { return this; };
+  }
+};
Index: lib/Sema/SemaLambda.cpp
===
--- lib/Sema/SemaLambda.cpp
+++ lib/Sema/SemaLambda.cpp
@@ -993,6 +993,8 @@
   CheckCXXThisCapture(C->Loc, /*Explicit=*/true, /*BuildAndDiagnose*/ true,
   /*FunctionScopeIndexToStopAtPtr*/ nullptr,
   C->Kind == LCK_StarThis);
+  LSI->ExplicitCaptureRanges[LSI->Captures.size() - 1] =
+  SourceRange(C->ExplicitLocStart, C->ExplicitLocEnd);
   continue;
 }
 
@@ -1139,6 +1141,8 @@
TryCapture_ExplicitByVal;
   tryCaptureVariable(Var, C->Loc, Kind, EllipsisLoc);
 }
+LSI->ExplicitCaptureRanges[LSI->Captures.size() - 1] =
+SourceRange(C->ExplicitLocStart, C->ExplicitLocEnd);
   }
   finishLambdaExplicitCaptures(LSI);
 
@@ -1478,19 +1482,22 @@
   return false;
 }
 
-void Sema::DiagnoseUnusedLambdaCapture(const Capture ) {
+bool Sema::DiagnoseUnusedLambdaCapture(const SourceRange CaptureRange,
+   const Capture ) {
   if (CaptureHasSideEffects(From))
-return;
+return false;
 
   if (From.isVLATypeCapture())
-return;
+return false;
 
   auto diag = Diag(From.getLocation(), diag::warn_unused_lambda_capture);
   if (From.isThisCapture())
 diag << "'this'";
   else
 diag << From.getVariable();
   diag << From.isNonODRUsed();
+  diag << FixItHint::CreateRemoval(CaptureRange);
+  return true;
 }
 
 ExprResult Sema::BuildLambdaExpr(SourceLocation StartLoc, SourceLocation EndLoc,
@@ -1532,18 +1539,49 @@
 
 // Translate captures.
 auto CurField = Class->field_begin();
+// True if the current capture has a used capture or default before it.
+bool CurHasPreviousCapture = CaptureDefault != LCD_None;
+SourceLocation PrevCaptureLoc = 

[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Andrew Comminos via Phabricator via cfe-commits]

acomminos updated this revision to Diff 155430.
acomminos marked 2 inline comments as done.
acomminos added a comment.

Elide braces in single-line conditional.


Repository:
  rC Clang

https://reviews.llvm.org/D48845

Files:
  include/clang/Sema/DeclSpec.h
  include/clang/Sema/ScopeInfo.h
  include/clang/Sema/Sema.h
  lib/Parse/ParseExprCXX.cpp
  lib/Sema/SemaLambda.cpp
  test/FixIt/fixit-unused-lambda-capture.cpp

Index: test/FixIt/fixit-unused-lambda-capture.cpp
===
--- /dev/null
+++ test/FixIt/fixit-unused-lambda-capture.cpp
@@ -0,0 +1,88 @@
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wunused-lambda-capture -Wno-unused-value -std=c++1z -fixit %t
+// RUN: grep -v CHECK %t | FileCheck %s
+
+void test() {
+  int i = 0;
+  int j = 0;
+  int k = 0;
+  int c = 10;
+  int a[c];
+
+  [i,j] { return i; };
+  // CHECK: [i] { return i; };
+  [i,j] { return j; };
+  // CHECK: [j] { return j; };
+  [,j] { return j; };
+  // CHECK: [j] { return j; };
+  [j,] { return j; };
+  // CHECK: [j] { return j; };
+  [i,j,k] {};
+  // CHECK: [] {};
+  [i,j,k] { return i + j; };
+  // CHECK: [i,j] { return i + j; };
+  [i,j,k] { return j + k; };
+  // CHECK: [j,k] { return j + k; };
+  [i,j,k] { return i + k; };
+  // CHECK: [i,k] { return i + k; };
+  [i,j,k] { return i + j + k; };
+  // CHECK: [i,j,k] { return i + j + k; };
+  [&,i] { return k; };
+  // CHECK: [&] { return k; };
+  [=,] { return k; };
+  // CHECK: [=] { return k; };
+  [=,,] { return j; };
+  // CHECK: [=,] { return j; };
+  [=,,] { return i; };
+  // CHECK: [=,] { return i; };
+  [z = i] {};
+  // CHECK: [] {};
+  [i,z = i] { return z; };
+  // CHECK: [z = i] { return z; };
+  [z = i,i] { return z; };
+  // CHECK: [z = i] { return z; };
+  [] {};
+  // CHECK: [] {};
+  [i,] { return i; };
+  // CHECK: [i] { return i; };
+  [,i] { return i; };
+  // CHECK: [i] { return i; };
+
+  #define I_MACRO() i
+  #define I_REF_MACRO() 
+  [I_MACRO()] {};
+  // CHECK: [] {};
+  [I_MACRO(),j] { return j; };
+  // CHECK: [j] { return j; };
+  [j,I_MACRO()] { return j; };
+  // CHECK: [j] { return j; };
+  [I_REF_MACRO(),j] { return j; };
+  // CHECK: [j] { return j; };
+  [j,I_REF_MACRO()] { return j; };
+  // CHECK: [j] { return j; };
+}
+
+class ThisTest {
+  void test() {
+int i = 0;
+
+[this] {};
+// CHECK: [] {};
+[i,this] { return i; };
+// CHECK: [i] { return i; };
+[this,i] { return i; };
+// CHECK: [i] { return i; };
+[*this] {};
+// CHECK: [] {};
+[*this,i] { return i; };
+// CHECK: [i] { return i; };
+[i,*this] { return i; };
+// CHECK: [i] { return i; };
+[*this] { return this; };
+// CHECK: [*this] { return this; };
+[*this,i] { return this; };
+// CHECK: [*this] { return this; };
+[i,*this] { return this; };
+// CHECK: [*this] { return this; };
+  }
+};
Index: lib/Sema/SemaLambda.cpp
===
--- lib/Sema/SemaLambda.cpp
+++ lib/Sema/SemaLambda.cpp
@@ -993,6 +993,8 @@
   CheckCXXThisCapture(C->Loc, /*Explicit=*/true, /*BuildAndDiagnose*/ true,
   /*FunctionScopeIndexToStopAtPtr*/ nullptr,
   C->Kind == LCK_StarThis);
+  LSI->ExplicitCaptureRanges[LSI->Captures.size() - 1] =
+  SourceRange(C->ExplicitLocStart, C->ExplicitLocEnd);
   continue;
 }
 
@@ -1139,6 +1141,8 @@
TryCapture_ExplicitByVal;
   tryCaptureVariable(Var, C->Loc, Kind, EllipsisLoc);
 }
+LSI->ExplicitCaptureRanges[LSI->Captures.size() - 1] =
+SourceRange(C->ExplicitLocStart, C->ExplicitLocEnd);
   }
   finishLambdaExplicitCaptures(LSI);
 
@@ -1478,7 +1482,8 @@
   return false;
 }
 
-void Sema::DiagnoseUnusedLambdaCapture(const Capture ) {
+void Sema::DiagnoseUnusedLambdaCapture(const SourceRange CaptureRange,
+   const Capture ) {
   if (CaptureHasSideEffects(From))
 return;
 
@@ -1491,6 +1496,7 @@
   else
 diag << From.getVariable();
   diag << From.isNonODRUsed();
+  diag << FixItHint::CreateRemoval(CaptureRange);
 }
 
 ExprResult Sema::BuildLambdaExpr(SourceLocation StartLoc, SourceLocation EndLoc,
@@ -1532,20 +1538,51 @@
 
 // Translate captures.
 auto CurField = Class->field_begin();
+// True if the current capture has a used capture or default before it.
+bool CurHasPreviousCapture = CaptureDefault != LCD_None;
+SourceLocation PrevCaptureLoc = CurHasPreviousCapture ?
+CaptureDefaultLoc : IntroducerRange.getBegin();
+
 for (unsigned I = 0, N = LSI->Captures.size(); I != N; ++I, ++CurField) {
   const Capture  = LSI->Captures[I];
+
   assert(!From.isBlockCapture() && "Cannot capture __block variables");
   bool IsImplicit = I >= LSI->NumExplicitCaptures;
 
+  // Use source ranges of explicit captures for fixits where available.
+  SourceRange 

[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Andrew Comminos via Phabricator via cfe-commits]

acomminos updated this revision to Diff 155428.
acomminos marked 2 inline comments as done.
acomminos added a comment.

Thanks! Updated to be more explicit about location names, add more tests for 
VLA and *this captures, and fix an issue with VLA range captures invalidating 
the capture range tracking.


Repository:
  rC Clang

https://reviews.llvm.org/D48845

Files:
  include/clang/Sema/DeclSpec.h
  include/clang/Sema/ScopeInfo.h
  include/clang/Sema/Sema.h
  lib/Parse/ParseExprCXX.cpp
  lib/Sema/SemaLambda.cpp
  test/FixIt/fixit-unused-lambda-capture.cpp

Index: test/FixIt/fixit-unused-lambda-capture.cpp
===
--- /dev/null
+++ test/FixIt/fixit-unused-lambda-capture.cpp
@@ -0,0 +1,88 @@
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wunused-lambda-capture -Wno-unused-value -std=c++1z -fixit %t
+// RUN: grep -v CHECK %t | FileCheck %s
+
+void test() {
+  int i = 0;
+  int j = 0;
+  int k = 0;
+  int c = 10;
+  int a[c];
+
+  [i,j] { return i; };
+  // CHECK: [i] { return i; };
+  [i,j] { return j; };
+  // CHECK: [j] { return j; };
+  [,j] { return j; };
+  // CHECK: [j] { return j; };
+  [j,] { return j; };
+  // CHECK: [j] { return j; };
+  [i,j,k] {};
+  // CHECK: [] {};
+  [i,j,k] { return i + j; };
+  // CHECK: [i,j] { return i + j; };
+  [i,j,k] { return j + k; };
+  // CHECK: [j,k] { return j + k; };
+  [i,j,k] { return i + k; };
+  // CHECK: [i,k] { return i + k; };
+  [i,j,k] { return i + j + k; };
+  // CHECK: [i,j,k] { return i + j + k; };
+  [&,i] { return k; };
+  // CHECK: [&] { return k; };
+  [=,] { return k; };
+  // CHECK: [=] { return k; };
+  [=,,] { return j; };
+  // CHECK: [=,] { return j; };
+  [=,,] { return i; };
+  // CHECK: [=,] { return i; };
+  [z = i] {};
+  // CHECK: [] {};
+  [i,z = i] { return z; };
+  // CHECK: [z = i] { return z; };
+  [z = i,i] { return z; };
+  // CHECK: [z = i] { return z; };
+  [] {};
+  // CHECK: [] {};
+  [i,] { return i; };
+  // CHECK: [i] { return i; };
+  [,i] { return i; };
+  // CHECK: [i] { return i; };
+
+  #define I_MACRO() i
+  #define I_REF_MACRO() 
+  [I_MACRO()] {};
+  // CHECK: [] {};
+  [I_MACRO(),j] { return j; };
+  // CHECK: [j] { return j; };
+  [j,I_MACRO()] { return j; };
+  // CHECK: [j] { return j; };
+  [I_REF_MACRO(),j] { return j; };
+  // CHECK: [j] { return j; };
+  [j,I_REF_MACRO()] { return j; };
+  // CHECK: [j] { return j; };
+}
+
+class ThisTest {
+  void test() {
+int i = 0;
+
+[this] {};
+// CHECK: [] {};
+[i,this] { return i; };
+// CHECK: [i] { return i; };
+[this,i] { return i; };
+// CHECK: [i] { return i; };
+[*this] {};
+// CHECK: [] {};
+[*this,i] { return i; };
+// CHECK: [i] { return i; };
+[i,*this] { return i; };
+// CHECK: [i] { return i; };
+[*this] { return this; };
+// CHECK: [*this] { return this; };
+[*this,i] { return this; };
+// CHECK: [*this] { return this; };
+[i,*this] { return this; };
+// CHECK: [*this] { return this; };
+  }
+};
Index: lib/Sema/SemaLambda.cpp
===
--- lib/Sema/SemaLambda.cpp
+++ lib/Sema/SemaLambda.cpp
@@ -993,6 +993,8 @@
   CheckCXXThisCapture(C->Loc, /*Explicit=*/true, /*BuildAndDiagnose*/ true,
   /*FunctionScopeIndexToStopAtPtr*/ nullptr,
   C->Kind == LCK_StarThis);
+  LSI->ExplicitCaptureRanges[LSI->Captures.size() - 1] =
+  SourceRange(C->ExplicitLocStart, C->ExplicitLocEnd);
   continue;
 }
 
@@ -1139,6 +1141,8 @@
TryCapture_ExplicitByVal;
   tryCaptureVariable(Var, C->Loc, Kind, EllipsisLoc);
 }
+LSI->ExplicitCaptureRanges[LSI->Captures.size() - 1] =
+SourceRange(C->ExplicitLocStart, C->ExplicitLocEnd);
   }
   finishLambdaExplicitCaptures(LSI);
 
@@ -1478,7 +1482,8 @@
   return false;
 }
 
-void Sema::DiagnoseUnusedLambdaCapture(const Capture ) {
+void Sema::DiagnoseUnusedLambdaCapture(const SourceRange CaptureRange,
+   const Capture ) {
   if (CaptureHasSideEffects(From))
 return;
 
@@ -1491,6 +1496,7 @@
   else
 diag << From.getVariable();
   diag << From.isNonODRUsed();
+  diag << FixItHint::CreateRemoval(CaptureRange);
 }
 
 ExprResult Sema::BuildLambdaExpr(SourceLocation StartLoc, SourceLocation EndLoc,
@@ -1532,18 +1538,50 @@
 
 // Translate captures.
 auto CurField = Class->field_begin();
+// True if the current capture has a used capture or default before it.
+bool CurHasPreviousCapture = CaptureDefault != LCD_None;
+SourceLocation PrevCaptureLoc = CurHasPreviousCapture ?
+CaptureDefaultLoc : IntroducerRange.getBegin();
+
 for (unsigned I = 0, N = LSI->Captures.size(); I != N; ++I, ++CurField) {
   const Capture  = LSI->Captures[I];
+
   assert(!From.isBlockCapture() && "Cannot capture __block variables");
   

[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/Sema/DeclSpec.h:2552-2553
 ParsedType InitCaptureType;
+SourceLocation LocStart;
+SourceLocation LocEnd;
+

How does `LocStart` relate to the existing source location `Loc`? I think this 
should have a more descriptive name of what location is involved.



Comment at: lib/Sema/SemaLambda.cpp:827
   Var->getType(), Var->getInit());
+
   return Field;

Spurious whitespace change can be removed.



Comment at: lib/Sema/SemaLambda.cpp:1553
+  SourceRange CaptureRange = LSI->ExplicitCaptureRanges[I];
+  if (CaptureRange.isInvalid()) {
+CaptureRange = SourceRange(From.getLocation());

Can elide braces.



Comment at: test/FixIt/fixit-unused-lambda-capture.cpp:64-66
+[*this] {};
+// CHECK: [] {};
+[*this,i] { return i; };

I'd like to see some tests where the `this` and `*this` captures are not 
removed by the fix.


Repository:
  rC Clang

https://reviews.llvm.org/D48845



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

[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Andrew Comminos via Phabricator via cfe-commits]

acomminos updated this revision to Diff 155246.
acomminos added a comment.

Thanks for the feedback! This diff switches to using a source range for 
captures provided by the parser, which is more accurate, future-proof, and 
correctly handles macros.


Repository:
  rC Clang

https://reviews.llvm.org/D48845

Files:
  include/clang/Sema/DeclSpec.h
  include/clang/Sema/ScopeInfo.h
  include/clang/Sema/Sema.h
  lib/Parse/ParseExprCXX.cpp
  lib/Sema/SemaLambda.cpp
  test/FixIt/fixit-unused-lambda-capture.cpp

Index: test/FixIt/fixit-unused-lambda-capture.cpp
===
--- /dev/null
+++ test/FixIt/fixit-unused-lambda-capture.cpp
@@ -0,0 +1,71 @@
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wunused-lambda-capture -std=c++1z -fixit %t
+// RUN: grep -v CHECK %t | FileCheck %s
+
+void test() {
+  int i = 0;
+  int j = 0;
+  int k = 0;
+  int c = 10;
+  int a[c];
+
+  [i,j] { return i; };
+  // CHECK: [i] { return i; };
+  [i,j] { return j; };
+  // CHECK: [j] { return j; };
+  [i,j,k] {};
+  // CHECK: [] {};
+  [i,j,k] { return i + j; };
+  // CHECK: [i,j] { return i + j; };
+  [i,j,k] { return j + k; };
+  // CHECK: [j,k] { return j + k; };
+  [i,j,k] { return i + k; };
+  // CHECK: [i,k] { return i + k; };
+  [i,j,k] { return i + j + k; };
+  // CHECK: [i,j,k] { return i + j + k; };
+  [&,i] { return k; };
+  // CHECK: [&] { return k; };
+  [=,] { return k; };
+  // CHECK: [=] { return k; };
+  [=,,] { return j; };
+  // CHECK: [=,] { return j; };
+  [=,,] { return i; };
+  // CHECK: [=,] { return i; };
+  [z = i] {};
+  // CHECK: [] {};
+  [i,z = i,j] { return z; };
+  // CHECK: [z = i] { return z; };
+  [] {};
+  // CHECK: [] {};
+
+  #define I_MACRO() i
+  #define I_REF_MACRO() 
+  [I_MACRO()] {};
+  // CHECK: [] {};
+  [I_MACRO(),j] { return j; };
+  // CHECK: [j] { return j; };
+  [j,I_MACRO()] { return j; };
+  // CHECK: [j] { return j; };
+  [I_REF_MACRO(),j] { return j; };
+  // CHECK: [j] { return j; };
+  [j,I_REF_MACRO()] { return j; };
+  // CHECK: [j] { return j; };
+}
+
+class ThisTest {
+  void test() {
+int i = 0;
+[this] {};
+// CHECK: [] {};
+[i,this] { return i; };
+// CHECK: [i] { return i; };
+[this,i] { return i; };
+// CHECK: [i] { return i; };
+[*this] {};
+// CHECK: [] {};
+[*this,i] { return i; };
+// CHECK: [i] { return i; };
+[i,*this] { return i; };
+// CHECK: [i] { return i; };
+  }
+};
Index: lib/Sema/SemaLambda.cpp
===
--- lib/Sema/SemaLambda.cpp
+++ lib/Sema/SemaLambda.cpp
@@ -824,6 +824,7 @@
   LSI->addCapture(Var, /*isBlock*/false, Var->getType()->isReferenceType(),
   /*isNested*/false, Var->getLocation(), SourceLocation(),
   Var->getType(), Var->getInit());
+
   return Field;
 }
 
@@ -954,6 +955,9 @@
 = Intro.Default == LCD_None? Intro.Range.getBegin() : Intro.DefaultLoc;
   for (auto C = Intro.Captures.begin(), E = Intro.Captures.end(); C != E;
PrevCaptureLoc = C->Loc, ++C) {
+LSI->ExplicitCaptureRanges[C - Intro.Captures.begin()] =
+SourceRange(C->LocStart, C->LocEnd);
+
 if (C->Kind == LCK_This || C->Kind == LCK_StarThis) {
   if (C->Kind == LCK_StarThis) 
 Diag(C->Loc, !getLangOpts().CPlusPlus17
@@ -1478,7 +1482,8 @@
   return false;
 }
 
-void Sema::DiagnoseUnusedLambdaCapture(const Capture ) {
+void Sema::DiagnoseUnusedLambdaCapture(const SourceRange CaptureRange,
+   const Capture ) {
   if (CaptureHasSideEffects(From))
 return;
 
@@ -1491,6 +1496,7 @@
   else
 diag << From.getVariable();
   diag << From.isNonODRUsed();
+  diag << FixItHint::CreateRemoval(CaptureRange);
 }
 
 ExprResult Sema::BuildLambdaExpr(SourceLocation StartLoc, SourceLocation EndLoc,
@@ -1532,19 +1538,47 @@
 
 // Translate captures.
 auto CurField = Class->field_begin();
+// True if the current capture has a used capture or default before it.
+bool CurHasPreviousCapture = CaptureDefault != LCD_None;
+SourceLocation PrevCaptureLoc = CurHasPreviousCapture ?
+CaptureDefaultLoc : IntroducerRange.getBegin();
+
 for (unsigned I = 0, N = LSI->Captures.size(); I != N; ++I, ++CurField) {
   const Capture  = LSI->Captures[I];
   assert(!From.isBlockCapture() && "Cannot capture __block variables");
   bool IsImplicit = I >= LSI->NumExplicitCaptures;
 
+  // Use source ranges of explicit captures for fixits where available.
+  SourceRange CaptureRange = LSI->ExplicitCaptureRanges[I];
+  if (CaptureRange.isInvalid()) {
+CaptureRange = SourceRange(From.getLocation());
+  }
+
   // Warn about unused explicit captures.
+  bool IsCaptureUsed = true;
   if (!CurContext->isDependentContext() && !IsImplicit && !From.isODRUsed()) {
 // Initialized captures that are non-ODR used may not be eliminated.
 bool 

[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Alexander Shaposhnikov via Phabricator via cfe-commits]

alexshap added a comment.

> Are you talking about a more conservative warning or a more conservative 
> fixit? If it doesn't make sense for us to have a fixit for a particular 
> capture, does it make sense for us to have a warning for that >capture in the 
> first place?

to be honest i'm more concerned with the fixit (so basically to avoid breaking 
the code - especially if these modifications are applied at scale, the code 
might be get broken silently and will be hard to find later, so I'd start with 
handling only simple cases where it's a strictly positive change)) )

> It would be helpful to add some tests with macros to ensure that the logic 
> for how the removal range is computed can handle macros. (E.g. macro that 
> expands to a full/partial capture, lambda in a macro).

+1


Repository:
  rC Clang

https://reviews.llvm.org/D48845



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

[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

Thanks for working on this! Please upload the patch with the full context (git 
diff -U9). It helps the reviewers :)

In https://reviews.llvm.org/D48845#1158103, @alexshap wrote:

> I'm kind of interested in this fixit, but one thought which i have - probably 
> it should be more conservative (i.e. fix captures by reference, integral 
> types, etc) (since the code might rely on side-effects of 
> copy-ctors/move-ctors or extend the lifetime of an object), but fixing only 
> simple cases still seems to be useful imo. CC: @aaron.ballman , @arphaman, 
> @ahatanak


Are you talking about a more conservative warning or a more conservative fixit? 
If it doesn't make sense for us to have a fixit for a particular capture, does 
it make sense for us to have a warning for that capture in the first place?

It would be helpful to add some tests with macros to ensure that the logic for 
how the removal range is computed can handle macros. (E.g. macro that expands 
to a full/partial capture, lambda in a macro).


Repository:
  rC Clang

https://reviews.llvm.org/D48845



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

[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Alexander Shaposhnikov via Phabricator via cfe-commits]

alexshap added a comment.

I'm kind of interested in this fixit, but one thought which i have - probably 
it should be more conservative (i.e. fix captures by reference, integral types, 
etc) (since the code might rely on side-effects of copy-ctors/move-ctors or 
extend the lifetime of an object), but fixing only simple cases still seems to 
be useful imo. CC: @aaron.ballman , @arphaman, @ahatanak


Repository:
  rC Clang

https://reviews.llvm.org/D48845



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

[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Alexander Shaposhnikov via Phabricator via cfe-commits]

alexshap added inline comments.



Comment at: lib/Sema/SemaLambda.cpp:1548
+  // Find the end of the explicit capture for use in fixits.
+  SourceLocation EndLoc;
+  if (From.isThisCapture() && From.isCopyCapture()) {

alexshap wrote:
> maybe these lines 1548 -1559 can be factored out into a helper function ?
+ maybe use a different name (EndLoc feels too generic in this particular 
case), but i don't insist


Repository:
  rC Clang

https://reviews.llvm.org/D48845



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

[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Alexander Shaposhnikov via Phabricator via cfe-commits]

alexshap added inline comments.



Comment at: lib/Sema/SemaLambda.cpp:1548
+  // Find the end of the explicit capture for use in fixits.
+  SourceLocation EndLoc;
+  if (From.isThisCapture() && From.isCopyCapture()) {

maybe these lines 1548 -1559 can be factored out into a helper function ?


Repository:
  rC Clang

https://reviews.llvm.org/D48845



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

[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Andrew Comminos via Phabricator via cfe-commits]

acomminos updated this revision to Diff 154313.
acomminos added a comment.

Add additional tests to ensure that explicit capture ranges are predicted 
correctly.


Repository:
  rC Clang

https://reviews.llvm.org/D48845

Files:
  include/clang/Sema/Sema.h
  lib/Sema/SemaLambda.cpp
  test/FixIt/fixit-unused-lambda-capture.cpp

Index: test/FixIt/fixit-unused-lambda-capture.cpp
===
--- /dev/null
+++ test/FixIt/fixit-unused-lambda-capture.cpp
@@ -0,0 +1,58 @@
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wunused-lambda-capture -std=c++1z -fixit %t
+// RUN: grep -v CHECK %t | FileCheck %s
+
+void test() {
+  int i = 0;
+  int j = 0;
+  int k = 0;
+  int c = 10;
+  int a[c];
+
+  [i,j] { return i; };
+  // CHECK: [i] { return i; };
+  [i,j] { return j; };
+  // CHECK: [j] { return j; };
+  [i,j,k] {};
+  // CHECK: [] {};
+  [i,j,k] { return i + j; };
+  // CHECK: [i,j] { return i + j; };
+  [i,j,k] { return j + k; };
+  // CHECK: [j,k] { return j + k; };
+  [i,j,k] { return i + k; };
+  // CHECK: [i,k] { return i + k; };
+  [i,j,k] { return i + j + k; };
+  // CHECK: [i,j,k] { return i + j + k; };
+  [&,i] { return k; };
+  // CHECK: [&] { return k; };
+  [=,] { return k; };
+  // CHECK: [=] { return k; };
+  [=,,] { return j; };
+  // CHECK: [=,] { return j; };
+  [=,,] { return i; };
+  // CHECK: [=,] { return i; };
+  [z = i] {};
+  // CHECK: [] {};
+  [i,z = i,j] { return z; };
+  // CHECK: [z = i] { return z; };
+  [] {};
+  // CHECK: [] {};
+}
+
+class ThisTest {
+  void test() {
+int i = 0;
+[this] {};
+// CHECK: [] {};
+[i,this] { return i; };
+// CHECK: [i] { return i; };
+[this,i] { return i; };
+// CHECK: [i] { return i; };
+[*this] {};
+// CHECK: [] {};
+[*this,i] { return i; };
+// CHECK: [i] { return i; };
+[i,*this] { return i; };
+// CHECK: [i] { return i; };
+  }
+};
Index: lib/Sema/SemaLambda.cpp
===
--- lib/Sema/SemaLambda.cpp
+++ lib/Sema/SemaLambda.cpp
@@ -1478,7 +1478,8 @@
   return false;
 }
 
-void Sema::DiagnoseUnusedLambdaCapture(const Capture ) {
+void Sema::DiagnoseUnusedLambdaCapture(const SourceRange CaptureRange,
+   const Capture ) {
   if (CaptureHasSideEffects(From))
 return;
 
@@ -1491,6 +1492,7 @@
   else
 diag << From.getVariable();
   diag << From.isNonODRUsed();
+  diag << FixItHint::CreateRemoval(CaptureRange);
 }
 
 ExprResult Sema::BuildLambdaExpr(SourceLocation StartLoc, SourceLocation EndLoc,
@@ -1532,19 +1534,51 @@
 
 // Translate captures.
 auto CurField = Class->field_begin();
+// True if the current capture has an initializer or default before it.
+bool CurHasPreviousInitializer = CaptureDefault != LCD_None;
+SourceLocation PrevCaptureLoc = CurHasPreviousInitializer ?
+CaptureDefaultLoc : IntroducerRange.getBegin();
+
 for (unsigned I = 0, N = LSI->Captures.size(); I != N; ++I, ++CurField) {
   const Capture  = LSI->Captures[I];
   assert(!From.isBlockCapture() && "Cannot capture __block variables");
   bool IsImplicit = I >= LSI->NumExplicitCaptures;
 
+  // Find the end of the explicit capture for use in fixits.
+  SourceLocation EndLoc;
+  if (From.isThisCapture() && From.isCopyCapture()) {
+// Skip dereference token in *this.
+EndLoc = getLocForEndOfToken(From.getLocation());
+  } else if (!From.isVLATypeCapture() && From.getInitExpr()) {
+// For initialized captures, use the end of the expression.
+EndLoc = From.getInitExpr()->getLocEnd();
+  } else {
+// Otherwise, use the location of the identifier token.
+EndLoc = From.getLocation();
+  }
+
   // Warn about unused explicit captures.
+  bool IsCaptureUsed = true;
   if (!CurContext->isDependentContext() && !IsImplicit && !From.isODRUsed()) {
 // Initialized captures that are non-ODR used may not be eliminated.
 bool NonODRUsedInitCapture =
 IsGenericLambda && From.isNonODRUsed() && From.getInitExpr();
-if (!NonODRUsedInitCapture)
-  DiagnoseUnusedLambdaCapture(From);
+if (!NonODRUsedInitCapture) {
+  // Include either the previous, next, or no comma to produce
+  // individually valid fixits, depending on the capture position.
+  SourceRange FixItRange;
+  bool IsLast = I + 1 == LSI->NumExplicitCaptures;
+  if (!CurHasPreviousInitializer && !IsLast) {
+FixItRange = SourceRange(From.getLocation(), getLocForEndOfToken(EndLoc));
+  } else {
+FixItRange = SourceRange(getLocForEndOfToken(PrevCaptureLoc), EndLoc);
+  }
+
+  DiagnoseUnusedLambdaCapture(FixItRange, From);
+  IsCaptureUsed = false;
+}
   }
+  CurHasPreviousInitializer |= IsCaptureUsed;
 
   // Handle 'this' capture.
   if 

[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Andrew Comminos via Phabricator via cfe-commits]

acomminos updated this revision to Diff 154307.
acomminos added a comment.

Handle initialization expressions and dereferenced `this` in lambda captures.

An alternative to handling various kinds of explicit captures would be 
propagating the source range for each lambda capture from the parser to each 
sema::Capture. This would only be applicable to explicit captures; is this 
preferable?


Repository:
  rC Clang

https://reviews.llvm.org/D48845

Files:
  include/clang/Sema/Sema.h
  lib/Sema/SemaLambda.cpp
  test/FixIt/fixit-unused-lambda-capture.cpp

Index: test/FixIt/fixit-unused-lambda-capture.cpp
===
--- /dev/null
+++ test/FixIt/fixit-unused-lambda-capture.cpp
@@ -0,0 +1,52 @@
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wunused-lambda-capture -std=c++1z -fixit %t
+// RUN: grep -v CHECK %t | FileCheck %s
+
+void test() {
+  int i = 0;
+  int j = 0;
+  int k = 0;
+  int c = 10;
+  int a[c];
+
+  [i,j] { return i; };
+  // CHECK: [i] { return i; };
+  [i,j] { return j; };
+  // CHECK: [j] { return j; };
+  [i,j,k] {};
+  // CHECK: [] {};
+  [i,j,k] { return i + j; };
+  // CHECK: [i,j] { return i + j; };
+  [i,j,k] { return j + k; };
+  // CHECK: [j,k] { return j + k; };
+  [i,j,k] { return i + k; };
+  // CHECK: [i,k] { return i + k; };
+  [i,j,k] { return i + j + k; };
+  // CHECK: [i,j,k] { return i + j + k; };
+  [&,i] { return k; };
+  // CHECK: [&] { return k; };
+  [=,] { return k; };
+  // CHECK: [=] { return k; };
+  [=,,] { return j; };
+  // CHECK: [=,] { return j; };
+  [=,,] { return i; };
+  // CHECK: [=,] { return i; };
+  [z = i] {};
+  // CHECK: [] {};
+  [i,z = i,j] { return z; };
+  // CHECK: [z = i] { return z; };
+  [] {};
+  // CHECK: [] {};
+}
+
+class ThisTest {
+  void test() {
+int i = 0;
+[this] {};
+// CHECK: [] {};
+[i,this] { return this; };
+// CHECK: [this] { return this; };
+[*this] {};
+// CHECK: [] {};
+  }
+};
Index: lib/Sema/SemaLambda.cpp
===
--- lib/Sema/SemaLambda.cpp
+++ lib/Sema/SemaLambda.cpp
@@ -1478,7 +1478,8 @@
   return false;
 }
 
-void Sema::DiagnoseUnusedLambdaCapture(const Capture ) {
+void Sema::DiagnoseUnusedLambdaCapture(const SourceRange CaptureRange,
+   const Capture ) {
   if (CaptureHasSideEffects(From))
 return;
 
@@ -1491,6 +1492,7 @@
   else
 diag << From.getVariable();
   diag << From.isNonODRUsed();
+  diag << FixItHint::CreateRemoval(CaptureRange);
 }
 
 ExprResult Sema::BuildLambdaExpr(SourceLocation StartLoc, SourceLocation EndLoc,
@@ -1532,19 +1534,51 @@
 
 // Translate captures.
 auto CurField = Class->field_begin();
+// True if the current capture has an initializer or default before it.
+bool CurHasPreviousInitializer = CaptureDefault != LCD_None;
+SourceLocation PrevCaptureLoc = CurHasPreviousInitializer ?
+CaptureDefaultLoc : IntroducerRange.getBegin();
+
 for (unsigned I = 0, N = LSI->Captures.size(); I != N; ++I, ++CurField) {
   const Capture  = LSI->Captures[I];
   assert(!From.isBlockCapture() && "Cannot capture __block variables");
   bool IsImplicit = I >= LSI->NumExplicitCaptures;
 
+  // Find the end of the explicit capture for use in fixits.
+  SourceLocation EndLoc;
+  if (From.isThisCapture() && From.isCopyCapture()) {
+// Skip dereference token in *this.
+EndLoc = getLocForEndOfToken(From.getLocation());
+  } else if (!From.isVLATypeCapture() && From.getInitExpr()) {
+// For initialized captures, use the end of the expression.
+EndLoc = From.getInitExpr()->getLocEnd();
+  } else {
+// Otherwise, use the location of the identifier token.
+EndLoc = From.getLocation();
+  }
+
   // Warn about unused explicit captures.
+  bool IsCaptureUsed = true;
   if (!CurContext->isDependentContext() && !IsImplicit && !From.isODRUsed()) {
 // Initialized captures that are non-ODR used may not be eliminated.
 bool NonODRUsedInitCapture =
 IsGenericLambda && From.isNonODRUsed() && From.getInitExpr();
-if (!NonODRUsedInitCapture)
-  DiagnoseUnusedLambdaCapture(From);
+if (!NonODRUsedInitCapture) {
+  // Delete either the preceding or next comma in the explicit capture
+  // list, depending on whether or not elements follow.
+  SourceRange FixItRange;
+  bool IsLast = I + 1 == LSI->NumExplicitCaptures;
+  if (!CurHasPreviousInitializer && !IsLast) {
+FixItRange = SourceRange(From.getLocation(), getLocForEndOfToken(EndLoc));
+  } else {
+FixItRange = SourceRange(getLocForEndOfToken(PrevCaptureLoc), EndLoc);
+  }
+
+  DiagnoseUnusedLambdaCapture(FixItRange, From);
+  IsCaptureUsed = false;
+}
   }
+  CurHasPreviousInitializer |= 

[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Andrew Comminos via Phabricator via cfe-commits]

acomminos planned changes to this revision.
acomminos added a comment.

Ah yes, thanks for pointing this out. Some additional logic is going to be 
necessary to handle capture initializers correctly- I'll look into exposing 
full source ranges in LambdaCapture to make this more consistent across capture 
types.


Repository:
  rC Clang

https://reviews.llvm.org/D48845



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

[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Benjamin Kramer via Phabricator via cfe-commits]

bkramer added inline comments.



Comment at: test/FixIt/fixit-unused-lambda-capture.cpp:31
+  // CHECK: [=,] { return i; };
+}

This needs tests for:

* capture initializers `[c = foo()] {};`
* Capturing this `[this] {};`
* Capturing *this `[*this] {};`
* VLA capture `int a; int c[a]; [] {};`


Repository:
  rC Clang

https://reviews.llvm.org/D48845



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

[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Andrew Comminos via Phabricator via cfe-commits]

acomminos updated this revision to Diff 153968.
acomminos retitled this revision from "[Sema] Add fixit for 
-Wno-unused-lambda-capture" to "[Sema] Add fixit for unused lambda captures".
acomminos edited the summary of this revision.
acomminos changed the visibility from "Custom Policy" to "Public (No Login 
Required)".
acomminos added a subscriber: alexshap.
acomminos added a comment.
Herald added a subscriber: cfe-commits.

Added tests, add logic for removing a comma forward for beginning edge case.


Repository:
  rC Clang

https://reviews.llvm.org/D48845

Files:
  include/clang/Sema/Sema.h
  lib/Sema/SemaLambda.cpp
  test/FixIt/fixit-unused-lambda-capture.cpp

Index: test/FixIt/fixit-unused-lambda-capture.cpp
===
--- /dev/null
+++ test/FixIt/fixit-unused-lambda-capture.cpp
@@ -0,0 +1,31 @@
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wunused-lambda-capture -std=c++1z -fixit %t
+// RUN: grep -v CHECK %t | FileCheck %s
+
+void test() {
+  int i = 0;
+  int j = 0;
+  int k = 0;
+  [i,j] { return i; };
+  // CHECK: [i] { return i; };
+  [i,j] { return j; };
+  // CHECK: [j] { return j; };
+  [i,j,k] {};
+  // CHECK: [] {};
+  [i,j,k] { return i + j; };
+  // CHECK: [i,j] { return i + j; };
+  [i,j,k] { return j + k; };
+  // CHECK: [j,k] { return j + k; };
+  [i,j,k] { return i + k; };
+  // CHECK: [i,k] { return i + k; };
+  [i,j,k] { return i + j + k; };
+  // CHECK: [i,j,k] { return i + j + k; };
+  [&,i] { return k; };
+  // CHECK: [&] { return k; };
+  [=,] { return k; };
+  // CHECK: [=] { return k; };
+  [=,,] { return j; };
+  // CHECK: [=,] { return j; };
+  [=,,] { return i; };
+  // CHECK: [=,] { return i; };
+}
Index: lib/Sema/SemaLambda.cpp
===
--- lib/Sema/SemaLambda.cpp
+++ lib/Sema/SemaLambda.cpp
@@ -1478,7 +1478,8 @@
   return false;
 }
 
-void Sema::DiagnoseUnusedLambdaCapture(const Capture ) {
+void Sema::DiagnoseUnusedLambdaCapture(const SourceRange CaptureRange,
+   const Capture ) {
   if (CaptureHasSideEffects(From))
 return;
 
@@ -1491,6 +1492,7 @@
   else
 diag << From.getVariable();
   diag << From.isNonODRUsed();
+  diag << FixItHint::CreateRemoval(CaptureRange);
 }
 
 ExprResult Sema::BuildLambdaExpr(SourceLocation StartLoc, SourceLocation EndLoc,
@@ -1532,19 +1534,40 @@
 
 // Translate captures.
 auto CurField = Class->field_begin();
+// True if the current capture has an initializer or default before it.
+bool CurHasPreviousInitializer = CaptureDefault != LCD_None;
+SourceLocation PrevCaptureLoc = CurHasPreviousInitializer ?
+CaptureDefaultLoc : IntroducerRange.getBegin();
+
 for (unsigned I = 0, N = LSI->Captures.size(); I != N; ++I, ++CurField) {
   const Capture  = LSI->Captures[I];
   assert(!From.isBlockCapture() && "Cannot capture __block variables");
   bool IsImplicit = I >= LSI->NumExplicitCaptures;
 
   // Warn about unused explicit captures.
+  bool IsCaptureUsed = true;
   if (!CurContext->isDependentContext() && !IsImplicit && !From.isODRUsed()) {
 // Initialized captures that are non-ODR used may not be eliminated.
 bool NonODRUsedInitCapture =
 IsGenericLambda && From.isNonODRUsed() && From.getInitExpr();
-if (!NonODRUsedInitCapture)
-  DiagnoseUnusedLambdaCapture(From);
+if (!NonODRUsedInitCapture) {
+  // Delete either the preceding or next comma in the explicit capture
+  // list, depending on whether or not elements follow.
+  SourceRange FixItRange;
+  bool IsLast = I + 1 == LSI->NumExplicitCaptures;
+  if (!CurHasPreviousInitializer && !IsLast) {
+FixItRange = SourceRange(From.getLocation(),
+ getLocForEndOfToken(From.getLocation()));
+  } else {
+FixItRange = SourceRange(getLocForEndOfToken(PrevCaptureLoc),
+ From.getLocation());
+  }
+
+  DiagnoseUnusedLambdaCapture(FixItRange, From);
+  IsCaptureUsed = false;
+}
   }
+  CurHasPreviousInitializer |= IsCaptureUsed;
 
   // Handle 'this' capture.
   if (From.isThisCapture()) {
@@ -1574,6 +1597,8 @@
 Init = InitResult.get();
   }
   CaptureInits.push_back(Init);
+
+  PrevCaptureLoc = From.getLocation();
 }
 
 // C++11 [expr.prim.lambda]p6:
Index: include/clang/Sema/Sema.h
===
--- include/clang/Sema/Sema.h
+++ include/clang/Sema/Sema.h
@@ -5604,7 +5604,8 @@
   bool CaptureHasSideEffects(const sema::Capture );
 
   /// Diagnose if an explicit lambda capture is unused.
-  void DiagnoseUnusedLambdaCapture(const sema::Capture );
+  void DiagnoseUnusedLambdaCapture(const SourceRange CaptureRange,
+   const