NoQ created this revision.
NoQ added reviewers: alexfh, gribozavr2, aaron.ballman, vsavchenko.
Herald added subscribers: martong, mgehre, xazax.hun.
NoQ requested review of this revision.
Herald added a project: clang-tools-extra.
This is a fairly basic form of aliasing that as we've noticed in D96215
<https://reviews.llvm.org/D96215> hasn't been covered yet.
At the same time this patch causes unfortunate false negatives on the
regression tests for `bugprone-redundant-branch-condition` that has a builtin
incomplete but slightly more robust solution to the same problem. These false
negatives are something that we didn't really have a right to warn at until
we've had a more flow-sensitive alias analysis. Like, they're valid warnings
but we can't emit them with the analysis that we have without consciously
introducing false positives as well. So I think it's a judgement call. I'm
definitely in favor of landing it despite the false negatives it introduces but
this may go against clang-tidy's philosophy on this issue so I'm open to not
landing it.
The patch uses `AnyCall` in order to cover 5 different AST node kinds for
call-like expressions none of which inherit from `CallExpr` (apart from
`CallExpr` itself).
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D101790
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
@@ -967,12 +967,29 @@
}
if (tryToExtinguish(onFire) && onFire) {
if (tryToExtinguishByVal(onFire) && onFire) {
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'onFire' [bugprone-redundant-branch-condition]
+ // NO-MESSAGE: technically tryToExtinguish() may launch
+ // a background thread to extinguish the fire while tryToExtinguishByVal()
+ // may be waiting for that thread to finish.
scream();
}
}
}
+bool &hidden_reference(bool &flag) {
+ return flag;
+}
+
+void test_hidden_reference() {
+ bool onFire = isBurning();
+ bool onFireRef = hidden_reference(onFire);
+ if (onFire) {
+ onFireRef = false;
+ if (onFire) {
+ // NO-MESSAGE: fire was extinguished by the above assignment
+ }
+ }
+}
+
void negative_reassigned() {
bool onFire = isBurning();
if (onFire) {
@@ -992,11 +1009,9 @@
bool onFire = isBurning();
if (onFire) {
if (onFire) {
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'onFire' [bugprone-redundant-branch-condition]
- // CHECK-FIXES: {{^\ *$}}
+ // FIXME: This should warn.
scream();
}
- // CHECK-FIXES: {{^\ *$}}
tryToExtinguish(onFire);
}
}
@@ -1006,11 +1021,9 @@
if (onFire) {
if (someOtherCondition()) {
if (onFire) {
- // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant condition 'onFire' [bugprone-redundant-branch-condition]
- // CHECK-FIXES: {{^\ *$}}
+ // FIXME: This should warn.
scream();
}
- // CHECK-FIXES: {{^\ *$}}
}
tryToExtinguish(onFire);
}
@@ -1021,11 +1034,9 @@
if (onFire) {
if (someOtherCondition()) {
if (onFire) {
- // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant condition 'onFire' [bugprone-redundant-branch-condition]
- // CHECK-FIXES: {{^\ *$}}
+ // FIXME: This should warn.
scream();
}
- // CHECK-FIXES: {{^\ *$}}
tryToExtinguish(onFire);
}
}
@@ -1035,8 +1046,7 @@
bool onFire = isBurning();
if (onFire) {
if (onFire) {
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'onFire' [bugprone-redundant-branch-condition]
- // CHECK-FIXES: {{^\ *$}}
+ // FIXME: This should warn.
tryToExtinguish(onFire);
scream();
}
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
@@ -551,3 +551,28 @@
do {
} while (false && CondVar);
}
+
+int &hidden_reference(int &x) {
+ return x;
+}
+
+void test_hidden_reference() {
+ int x = 0;
+ int &y = hidden_reference(x);
+ for (; x < 10; ++y) {
+ // No warning. The loop is finite because 'y' is a reference to 'x'.
+ }
+}
+
+struct HiddenReference {
+ int &y;
+ HiddenReference(int &x) : y(x) {}
+};
+
+void test_HiddenReference() {
+ int x = 0;
+ int &y = HiddenReference(x).y;
+ for (; x < 10; ++y) {
+ // No warning. The loop is finite because 'y' is a reference to 'x'.
+ }
+}
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
@@ -8,6 +8,7 @@
#include "Aliasing.h"
+#include "clang/Analysis/AnyCall.h"
#include "clang/AST/Expr.h"
#include "clang/AST/ExprCXX.h"
@@ -50,6 +51,10 @@
} else if (const auto *LE = dyn_cast<LambdaExpr>(S)) {
// Treat lambda capture by reference as a form of taking a reference.
return capturesByRef(LE->getLambdaClass(), Var);
+ } else if (Optional<AnyCall> Call = AnyCall::forExpr(S)) {
+ return llvm::any_of(Call->arguments(), [Var](const Expr *ArgE) {
+ return isAccessForVar(ArgE, Var);
+ });
}
return false;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
