ymandel created this revision.
ymandel added a reviewer: gribozavr.
Herald added a subscriber: xazax.hun.
Herald added a project: clang.
The bugprone-use-after-move check exhibits false positives for certain uses of
the C++17 if/switch init statements. These false positives are caused by a bug
in the ExprSequence calculations.
This revision adds tests for the false positives and fixes the corresponding
sequence calculation.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D67292
Files:
clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
clang-tools-extra/test/clang-tidy/bugprone-use-after-move.cpp
Index: clang-tools-extra/test/clang-tidy/bugprone-use-after-move.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/bugprone-use-after-move.cpp
+++ clang-tools-extra/test/clang-tidy/bugprone-use-after-move.cpp
@@ -1190,6 +1190,12 @@
std::move(a2);
}
}
+ for (int i = 0; i < 10; ++i) {
+ A a1;
+ if (A a2 = std::move(a1); a2) {
+ std::move(a2);
+ }
+ }
for (int i = 0; i < 10; ++i) {
A a1;
if (A a2 = std::move(a1); A a3 = std::move(a2)) {
@@ -1199,6 +1205,13 @@
while (A a = A()) {
std::move(a);
}
+ for (int i = 0; i < 10; ++i) {
+ A a1;
+ switch (A a2 = std::move(a1); a2) {
+ case true:
+ std::move(a2);
+ }
+ }
for (int i = 0; i < 10; ++i) {
A a1;
switch (A a2 = a1; A a3 = std::move(a2)) {
Index: clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
===================================================================
--- clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
+++ clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
@@ -145,17 +145,24 @@
return ForRange->getBody();
} else if (const auto *TheIfStmt = dyn_cast<IfStmt>(Parent)) {
// If statement:
- // - Sequence init statement before variable declaration.
+ // - Sequence init statement before variable declaration, if present;
+ // before condition evaluation, otherwise.
// - Sequence variable declaration (along with the expression used to
// initialize it) before the evaluation of the condition.
- if (S == TheIfStmt->getInit())
- return TheIfStmt->getConditionVariableDeclStmt();
+ if (S == TheIfStmt->getInit()) {
+ if (TheIfStmt->getConditionVariableDeclStmt() != nullptr)
+ return TheIfStmt->getConditionVariableDeclStmt();
+ return TheIfStmt->getCond();
+ }
if (S == TheIfStmt->getConditionVariableDeclStmt())
return TheIfStmt->getCond();
} else if (const auto *TheSwitchStmt = dyn_cast<SwitchStmt>(Parent)) {
// Ditto for switch statements.
- if (S == TheSwitchStmt->getInit())
- return TheSwitchStmt->getConditionVariableDeclStmt();
+ if (S == TheSwitchStmt->getInit()) {
+ if (TheSwitchStmt->getConditionVariableDeclStmt() != nullptr)
+ return TheSwitchStmt->getConditionVariableDeclStmt();
+ return TheSwitchStmt->getCond();
+ }
if (S == TheSwitchStmt->getConditionVariableDeclStmt())
return TheSwitchStmt->getCond();
} else if (const auto *TheWhileStmt = dyn_cast<WhileStmt>(Parent)) {
Index: clang-tools-extra/test/clang-tidy/bugprone-use-after-move.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/bugprone-use-after-move.cpp
+++ clang-tools-extra/test/clang-tidy/bugprone-use-after-move.cpp
@@ -1190,6 +1190,12 @@
std::move(a2);
}
}
+ for (int i = 0; i < 10; ++i) {
+ A a1;
+ if (A a2 = std::move(a1); a2) {
+ std::move(a2);
+ }
+ }
for (int i = 0; i < 10; ++i) {
A a1;
if (A a2 = std::move(a1); A a3 = std::move(a2)) {
@@ -1199,6 +1205,13 @@
while (A a = A()) {
std::move(a);
}
+ for (int i = 0; i < 10; ++i) {
+ A a1;
+ switch (A a2 = std::move(a1); a2) {
+ case true:
+ std::move(a2);
+ }
+ }
for (int i = 0; i < 10; ++i) {
A a1;
switch (A a2 = a1; A a3 = std::move(a2)) {
Index: clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
===================================================================
--- clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
+++ clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
@@ -145,17 +145,24 @@
return ForRange->getBody();
} else if (const auto *TheIfStmt = dyn_cast<IfStmt>(Parent)) {
// If statement:
- // - Sequence init statement before variable declaration.
+ // - Sequence init statement before variable declaration, if present;
+ // before condition evaluation, otherwise.
// - Sequence variable declaration (along with the expression used to
// initialize it) before the evaluation of the condition.
- if (S == TheIfStmt->getInit())
- return TheIfStmt->getConditionVariableDeclStmt();
+ if (S == TheIfStmt->getInit()) {
+ if (TheIfStmt->getConditionVariableDeclStmt() != nullptr)
+ return TheIfStmt->getConditionVariableDeclStmt();
+ return TheIfStmt->getCond();
+ }
if (S == TheIfStmt->getConditionVariableDeclStmt())
return TheIfStmt->getCond();
} else if (const auto *TheSwitchStmt = dyn_cast<SwitchStmt>(Parent)) {
// Ditto for switch statements.
- if (S == TheSwitchStmt->getInit())
- return TheSwitchStmt->getConditionVariableDeclStmt();
+ if (S == TheSwitchStmt->getInit()) {
+ if (TheSwitchStmt->getConditionVariableDeclStmt() != nullptr)
+ return TheSwitchStmt->getConditionVariableDeclStmt();
+ return TheSwitchStmt->getCond();
+ }
if (S == TheSwitchStmt->getConditionVariableDeclStmt())
return TheSwitchStmt->getCond();
} else if (const auto *TheWhileStmt = dyn_cast<WhileStmt>(Parent)) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
