NoQ created this revision.
NoQ added reviewers: alexfh, gribozavr2, aaron.ballman, vsavchenko.
Herald added subscribers: nullptr.cpp, martong, mgehre, Charusso, xazax.hun.
NoQ requested review of this revision.
The utility function `clang::tidy::utils::`()` scans the function for
pointer/reference aliases to a given variable. It currently scans for operator
`&` over that variable and for declarations of references to that variable.
I'm arguing that it should scan for lambda captures by reference as well. If a
lambda captures a variable by reference it basically has a reference to that
variable that the user of the lambda can access at any time, including on
background thread, which may have meaningful impact on checkers. The same
applies to blocks (an Apple extension that brings closures to C and
Objective-C; blocks are also implicitly convertible with lambdas when both
facilities are available).
The patch fixes false positives in both checkers that use this functionality:
`bugprone-infinite-loop` and `bugprone-redundant-branch-condition`. There are a
few false negatives it introduces as a tradeoff: if the lambda captures a
variable by reference but never actually mutates it we'll no longer warn but we
should (as demonstrated by FIXME tests). These false negatives are
architecturally annoying to deal with because `hasPtrOrReferenceInFunc()`
detects aliasing rather than mutation, so it'll have to be a different facility
entirely. They're also rudimentary because there's rarely a good reason to
write such code; it probably deserves a separate warning to relax capture kind
to a by-value or by-const-reference capture (which is often explicit). That
said, C++'s `[&]` would probably capture a lot of stuff by reference
unnecessarily and it'll often make their code worse if developers are forced to
write down all captures manually instead.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D96215
Files:
clang-tools-extra/clang-tidy/utils/Aliasing.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
@@ -1179,6 +1179,86 @@
}
}
+// Lambda / block captures.
+
+template <typename T> void accept_callback(T t) {
+ // Potentially call the callback.
+ // Possibly on a background thread or something.
+}
+
+void accept_block(void (^)(void)) {
+ // Potentially call the callback.
+ // Possibly on a background thread or something.
+}
+
+void wait(void) {
+ // Wait for the previously passed callback to be called.
+}
+
+void capture_and_mutate_by_lambda() {
+ bool x = true;
+ accept_callback([&]() { x = false; });
+ if (x) {
+ wait();
+ if (x) {
+ }
+ }
+}
+
+void lambda_capture_by_value() {
+ bool x = true;
+ accept_callback([x]() { if (x) {} });
+ if (x) {
+ wait();
+ if (x) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'x' [bugprone-redundant-branch-condition]
+ }
+ }
+}
+
+void capture_by_lambda_but_not_mutate() {
+ bool x = true;
+ accept_callback([&]() { if (x) {} });
+ if (x) {
+ wait();
+ // FIXME: Should warn.
+ if (x) {
+ }
+ }
+}
+
+void capture_and_mutate_by_block() {
+ __block bool x = true;
+ accept_block(^{ x = false; });
+ if (x) {
+ wait();
+ if (x) {
+ }
+ }
+}
+
+void block_capture_by_value() {
+ bool x = true;
+ accept_block(^{ if (x) {} });
+ if (x) {
+ wait();
+ if (x) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'x' [bugprone-redundant-branch-condition]
+ }
+ }
+}
+
+void capture_by_block_but_not_mutate() {
+ __block bool x = true;
+ accept_callback(^{ if (x) {} });
+ if (x) {
+ wait();
+ // FIXME: Should warn.
+ if (x) {
+ }
+ }
+}
+
// GNU Expression Statements
void negative_gnu_expression_statement() {
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
@@ -1,4 +1,5 @@
-// RUN: %check_clang_tidy %s bugprone-infinite-loop %t -- -- -fexceptions
+// RUN: %check_clang_tidy %s bugprone-infinite-loop %t -- -- \
+// RUN: -fexceptions -fblocks
void simple_infinite_loop1() {
int i = 0;
@@ -378,6 +379,84 @@
} while (i < Limit);
}
+template <typename T> void accept_callback(T t) {
+ // Potentially call the callback.
+ // Possibly on a background thread or something.
+}
+
+void accept_block(void (^)(void)) {
+ // Potentially call the callback.
+ // Possibly on a background thread or something.
+}
+
+void wait(void) {
+ // Wait for the previously passed callback to be called.
+}
+
+void lambda_capture_from_outside() {
+ bool finished = false;
+ accept_callback([&]() {
+ finished = true;
+ });
+ while (!finished) {
+ wait();
+ }
+}
+
+void lambda_capture_from_outside_by_value() {
+ bool finished = false;
+ accept_callback([finished]() {
+ if (finished) {}
+ });
+ while (!finished) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (finished) are updated in the loop body [bugprone-infinite-loop]
+ wait();
+ }
+}
+
+void lambda_capture_from_outside_but_unchanged() {
+ bool finished = false;
+ accept_callback([&finished]() {
+ if (finished) {}
+ });
+ while (!finished) {
+ // FIXME: Should warn.
+ wait();
+ }
+}
+
+void block_capture_from_outside() {
+ __block bool finished = false;
+ accept_block(^{
+ finished = true;
+ });
+ while (!finished) {
+ wait();
+ }
+}
+
+void block_capture_from_outside_by_value() {
+ bool finished = false;
+ accept_block(^{
+ if (finished) {}
+ });
+ while (!finished) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (finished) are updated in the loop body [bugprone-infinite-loop]
+ wait();
+ }
+}
+
+void block_capture_from_outside_but_unchanged() {
+ __block bool finished = false;
+ accept_block(^{
+ if (finished) {}
+ });
+ while (!finished) {
+ // FIXME: Should warn.
+ wait();
+ }
+}
+
void evaluatable(bool CondVar) {
for (; false && CondVar;) {
}
Index: clang-tools-extra/clang-tidy/utils/Aliasing.cpp
===================================================================
--- clang-tools-extra/clang-tidy/utils/Aliasing.cpp
+++ clang-tools-extra/clang-tidy/utils/Aliasing.cpp
@@ -9,6 +9,7 @@
#include "Aliasing.h"
#include "clang/AST/Expr.h"
+#include "clang/AST/ExprCXX.h"
namespace clang {
namespace tidy {
@@ -24,6 +25,10 @@
/// Return whether \p Var has a pointer or reference in \p S.
static bool isPtrOrReferenceForVar(const Stmt *S, const VarDecl *Var) {
+ // Treat block capture by reference as a form of taking a reference.
+ if (Var->isEscapingByref())
+ return true;
+
if (const auto *DS = dyn_cast<DeclStmt>(S)) {
for (const Decl *D : DS->getDeclGroup()) {
if (const auto *LeftVar = dyn_cast<VarDecl>(D)) {
@@ -35,6 +40,12 @@
} else if (const auto *UnOp = dyn_cast<UnaryOperator>(S)) {
if (UnOp->getOpcode() == UO_AddrOf)
return isAccessForVar(UnOp->getSubExpr(), Var);
+ } else if (const auto *LE = dyn_cast<LambdaExpr>(S)) {
+ // Treat lambda capture by reference as a form of taking a reference.
+ return llvm::any_of(LE->captures(), [Var](const LambdaCapture &C) {
+ return C.capturesVariable() && C.getCaptureKind() == LCK_ByRef &&
+ C.getCapturedVar() == Var;
+ });
}
return false;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
