[PATCH] D48845: [Sema] Add fixit for unused lambda captures
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 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
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&id=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 &From) { +bool Sema::DiagnoseUnusedLambdaCapture(SourceRange CaptureRange, + const Capture &From) { 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 &From = 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
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; }; + [&i,j] { return j; }; + // CHECK: [j] { return j; }; + [j,&i] { 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; }; + [=,&i] { return k; }; + // CHECK: [=] { return k; }; + [=,&i,&j] { return j; }; + // CHECK: [=,&j] { return j; }; + [=,&i,&j] { return i; }; + // CHECK: [=,&i] { 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; }; + [&a] {}; + // CHECK: [] {}; + [i,&a] { return i; }; + // CHECK: [i] { return i; }; + [&a,i] { return i; }; + // CHECK: [i] { return i; }; + + #define I_MACRO() i + #define I_REF_MACRO() &i + [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 &From) { +bool Sema::DiagnoseUnusedLambdaCapture(SourceRange CaptureRange, + const Capture &From) { 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 &From = LSI->Captures[I];
[PATCH] D48845: [Sema] Add fixit for unused lambda captures
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 &From); 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
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
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; }; + [&i,j] { return j; }; + // CHECK: [j] { return j; }; + [j,&i] { 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; }; + [=,&i] { return k; }; + // CHECK: [=] { return k; }; + [=,&i,&j] { return j; }; + // CHECK: [=,&j] { return j; }; + [=,&i,&j] { return i; }; + // CHECK: [=,&i] { 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; }; + [&a] {}; + // CHECK: [] {}; + [i,&a] { return i; }; + // CHECK: [i] { return i; }; + [&a,i] { return i; }; + // CHECK: [i] { return i; }; + + #define I_MACRO() i + #define I_REF_MACRO() &i + [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 &From) { +bool Sema::DiagnoseUnusedLambdaCapture(const SourceRange CaptureRange, + const Capture &From) { 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 = L
[PATCH] D48845: [Sema] Add fixit for unused lambda captures
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 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
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; }; + [&i,j] { return j; }; + // CHECK: [j] { return j; }; + [j,&i] { 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; }; + [=,&i] { return k; }; + // CHECK: [=] { return k; }; + [=,&i,&j] { return j; }; + // CHECK: [=,&j] { return j; }; + [=,&i,&j] { return i; }; + // CHECK: [=,&i] { 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; }; + [&a] {}; + // CHECK: [] {}; + [i,&a] { return i; }; + // CHECK: [i] { return i; }; + [&a,i] { return i; }; + // CHECK: [i] { return i; }; + + #define I_MACRO() i + #define I_REF_MACRO() &i + [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 &From) { +bool Sema::DiagnoseUnusedLambdaCapture(const SourceRange CaptureRange, + const Capture &From) { 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 Pr
[PATCH] D48845: [Sema] Add fixit for unused lambda captures
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; }; + [&i,j] { return j; }; + // CHECK: [j] { return j; }; + [j,&i] { 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; }; + [=,&i] { return k; }; + // CHECK: [=] { return k; }; + [=,&i,&j] { return j; }; + // CHECK: [=,&j] { return j; }; + [=,&i,&j] { return i; }; + // CHECK: [=,&i] { 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; }; + [&a] {}; + // CHECK: [] {}; + [i,&a] { return i; }; + // CHECK: [i] { return i; }; + [&a,i] { return i; }; + // CHECK: [i] { return i; }; + + #define I_MACRO() i + #define I_REF_MACRO() &i + [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 &From) { +void Sema::DiagnoseUnusedLambdaCapture(const SourceRange CaptureRange, + const Capture &From) { 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 &From = LSI->Captures[I]; + assert(!From.isBlockCapture() && "Cannot capture __block variables"); bool IsImplicit = I >= LSI->NumExplicitCaptures; + // Use source ranges of explicit captures for fix
[PATCH] D48845: [Sema] Add fixit for unused lambda captures
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; }; + [&i,j] { return j; }; + // CHECK: [j] { return j; }; + [j,&i] { 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; }; + [=,&i] { return k; }; + // CHECK: [=] { return k; }; + [=,&i,&j] { return j; }; + // CHECK: [=,&j] { return j; }; + [=,&i,&j] { return i; }; + // CHECK: [=,&i] { 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; }; + [&a] {}; + // CHECK: [] {}; + [i,&a] { return i; }; + // CHECK: [i] { return i; }; + [&a,i] { return i; }; + // CHECK: [i] { return i; }; + + #define I_MACRO() i + #define I_REF_MACRO() &i + [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 &From) { +void Sema::DiagnoseUnusedLambdaCapture(const SourceRange CaptureRange, + const Capture &From) { 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 &From = LSI->Captures[I]; + assert(!From.isBlockCapture() && "Cannot
[PATCH] D48845: [Sema] Add fixit for unused lambda captures
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
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; }; + [=,&i] { return k; }; + // CHECK: [=] { return k; }; + [=,&i,&j] { return j; }; + // CHECK: [=,&j] { return j; }; + [=,&i,&j] { return i; }; + // CHECK: [=,&i] { return i; }; + [z = i] {}; + // CHECK: [] {}; + [i,z = i,j] { return z; }; + // CHECK: [z = i] { return z; }; + [&a] {}; + // CHECK: [] {}; + + #define I_MACRO() i + #define I_REF_MACRO() &i + [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 &From) { +void Sema::DiagnoseUnusedLambdaCapture(const SourceRange CaptureRange, + const Capture &From) { 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 &From = 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.
[PATCH] D48845: [Sema] Add fixit for unused lambda captures
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
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
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
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
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
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; }; + [=,&i] { return k; }; + // CHECK: [=] { return k; }; + [=,&i,&j] { return j; }; + // CHECK: [=,&j] { return j; }; + [=,&i,&j] { return i; }; + // CHECK: [=,&i] { return i; }; + [z = i] {}; + // CHECK: [] {}; + [i,z = i,j] { return z; }; + // CHECK: [z = i] { return z; }; + [&a] {}; + // 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 &From) { +void Sema::DiagnoseUnusedLambdaCapture(const SourceRange CaptureRange, + const Capture &From) { 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 &From = 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; // Ha
[PATCH] D48845: [Sema] Add fixit for unused lambda captures
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; }; + [=,&i] { return k; }; + // CHECK: [=] { return k; }; + [=,&i,&j] { return j; }; + // CHECK: [=,&j] { return j; }; + [=,&i,&j] { return i; }; + // CHECK: [=,&i] { return i; }; + [z = i] {}; + // CHECK: [] {}; + [i,z = i,j] { return z; }; + // CHECK: [z = i] { return z; }; + [&a] {}; + // 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 &From) { +void Sema::DiagnoseUnusedLambdaCapture(const SourceRange CaptureRange, + const Capture &From) { 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 &From = 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; +} } + CurHasPrevio
[PATCH] D48845: [Sema] Add fixit for unused lambda captures
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
bkramer added inline comments. Comment at: test/FixIt/fixit-unused-lambda-capture.cpp:31 + // CHECK: [=,&i] { return i; }; +} This needs tests for: * capture initializers `[c = foo()] {};` * Capturing this `[this] {};` * Capturing *this `[*this] {};` * VLA capture `int a; int c[a]; [&c] {};` 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
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; }; + [=,&i] { return k; }; + // CHECK: [=] { return k; }; + [=,&i,&j] { return j; }; + // CHECK: [=,&j] { return j; }; + [=,&i,&j] { 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 &From) { +void Sema::DiagnoseUnusedLambdaCapture(const SourceRange CaptureRange, + const Capture &From) { 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 &From = 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 &From); /// Diagnose if an explicit lambda capture is unused. - void DiagnoseUnusedLambdaCapture(const sema::Capture &From); + void DiagnoseUnusedLambdaCapture(const SourceRange CaptureRange, +