Author: ymandel Date: Mon Sep 9 05:59:14 2019 New Revision: 371396 URL: http://llvm.org/viewvc/llvm-project?rev=371396&view=rev Log: [clang-tidy] Fix bug in bugprone-use-after-move check
Summary: 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. Reviewers: gribozavr Subscribers: xazax.hun, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D67292 Modified: clang-tools-extra/trunk/clang-tidy/utils/ExprSequence.cpp clang-tools-extra/trunk/test/clang-tidy/bugprone-use-after-move.cpp Modified: clang-tools-extra/trunk/clang-tidy/utils/ExprSequence.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/utils/ExprSequence.cpp?rev=371396&r1=371395&r2=371396&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/utils/ExprSequence.cpp (original) +++ clang-tools-extra/trunk/clang-tidy/utils/ExprSequence.cpp Mon Sep 9 05:59:14 2019 @@ -145,17 +145,24 @@ const Stmt *ExprSequence::getSequenceSuc 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)) { Modified: clang-tools-extra/trunk/test/clang-tidy/bugprone-use-after-move.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/bugprone-use-after-move.cpp?rev=371396&r1=371395&r2=371396&view=diff ============================================================================== --- clang-tools-extra/trunk/test/clang-tidy/bugprone-use-after-move.cpp (original) +++ clang-tools-extra/trunk/test/clang-tidy/bugprone-use-after-move.cpp Mon Sep 9 05:59:14 2019 @@ -1182,7 +1182,10 @@ void forRangeSequences() { // If a variable is declared in an if, while or switch statement, the init // statement (for if and switch) is sequenced before the variable declaration, -// which in turn is sequenced before the evaluation of the condition. +// which in turn is sequenced before the evaluation of the condition. We place +// all tests inside a for loop to ensure that the checker understands the +// sequencing. If it didn't, then the loop would trigger the "moved twice" +// logic. void ifWhileAndSwitchSequenceInitDeclAndCondition() { for (int i = 0; i < 10; ++i) { A a1; @@ -1192,15 +1195,44 @@ void ifWhileAndSwitchSequenceInitDeclAnd } 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)) { std::move(a3); } } + for (int i = 0; i < 10; ++i) { + // init followed by condition with move, but without variable declaration. + if (A a1; A(std::move(a1)).getInt() > 0) {} + } + for (int i = 0; i < 10; ++i) { + if (A a1; A(std::move(a1)).getInt() > a1.getInt()) {} + // CHECK-NOTES: [[@LINE-1]]:43: warning: 'a1' used after it was moved + // CHECK-NOTES: [[@LINE-2]]:15: note: move occurred here + // CHECK-NOTES: [[@LINE-3]]:43: note: the use and move are unsequenced + } + for (int i = 0; i < 10; ++i) { + A a1; + if (A a2 = std::move(a1); A(a1) > 0) {} + // CHECK-NOTES: [[@LINE-1]]:33: warning: 'a1' used after it was moved + // CHECK-NOTES: [[@LINE-2]]:16: note: move occurred here + } 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)) { case true: std::move(a3); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
