Index: clang/include/clang/Basic/DiagnosticSemaKinds.td =================================================================== --- clang/include/clang/Basic/DiagnosticSemaKinds.td +++ clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -27,6 +27,15 @@ "and in the loop body">, InGroup, DefaultIgnore; def note_loop_iteration_here : Note<"%select{decremented|incremented}0 here">; +def warn_loop_wrong_iteration : Warning< + "for-loop comparison has a %select{less|greater}0 than " + "%select{|or equal to }1operator (%2) but the loop step " + "%select{decrements|increments}3 the %select{left|right}4-hand side of the " + "comparison">, InGroup, DefaultIgnore; +def note_loop_wrong_iteration_flip_condition : Note< + "flip comparison to %select{greater than|less than}0%select{| or equal to}1">; +def note_loop_wrong_iteration_flip_step : Note< + "or use %select{increment|decrement}0">; def warn_duplicate_enum_values : Warning< "element %0 has been implicitly assigned %1 which another element has " Index: clang/lib/Sema/SemaStmt.cpp =================================================================== --- clang/lib/Sema/SemaStmt.cpp +++ clang/lib/Sema/SemaStmt.cpp @@ -1739,6 +1739,93 @@ << LoopIncrement; } + // Emit a warning when the increment is the opposite of what is implied + // by the condition. For instance, the condition 'x < y' implies the correct + // increment is either '++x' or '--y' and will generate a warning with + // 'for (; x < y; --x)' or for '(; x < y; ++y)' + void CheckWrongIterationDirection(Sema &SemaRef, const Expr *Second, + const Expr *Third) { + if (!Second || !Third) return; + + // Only check relational comparisons + const BinaryOperator *Cond = dyn_cast(Second); + if (!Cond) return; + if (!Cond->isRelationalOp()) return; + const auto Opcode = Cond->getOpcode(); + + // Extract the operands of the comparison. At least one needs to simple + // Decl. + const VarDecl *LeftOperand = nullptr; + const VarDecl *RightOperand = nullptr; + if (const DeclRefExpr *DRE = + dyn_cast(Cond->getLHS()->IgnoreParenImpCasts())) { + LeftOperand = dyn_cast(DRE->getDecl()); + } + if (const DeclRefExpr *DRE = + dyn_cast(Cond->getRHS()->IgnoreParenImpCasts())) { + RightOperand = dyn_cast(DRE->getDecl()); + } + if (!LeftOperand || !RightOperand) return; + if (LeftOperand == RightOperand) return; + + // Unsigned types have well-defined overflow and underflow behaviors, + // so skip checking them. + if (LeftOperand->getType()->isUnsignedIntegerType() || + RightOperand->getType()->isUnsignedIntegerType()) + return; + const bool IsConditionGreaterThan = Opcode == BO_GT || Opcode == BO_GE; + const bool IsOrEqual = Opcode == BO_GE || Opcode == BO_LE; + + // Check that the loop step is an increment/decrement operation. + const UnaryOperator* Increment = dyn_cast(Third); + if (!Increment) return; + if (!Increment->isIncrementDecrementOp()) return; + const VarDecl *IncrementOperand = nullptr; + if (const DeclRefExpr *DRE = dyn_cast( + Increment->getSubExpr()->IgnoreParenImpCasts())) { + IncrementOperand = dyn_cast(DRE->getDecl()); + } + if (!IncrementOperand) return; + const bool IsIncrement = Increment->isIncrementOp(); + + const bool IsLHSInIncrement = IncrementOperand == LeftOperand; + const bool IsRHSInIncrement = IncrementOperand == RightOperand; + + if (!IsLHSInIncrement && !IsRHSInIncrement) return; + + // for (; x > ...; x--) + if (IsLHSInIncrement && IsConditionGreaterThan && !IsIncrement) return; + // for (; x < ...; x++) + if (IsLHSInIncrement && !IsConditionGreaterThan && IsIncrement) return; + // for (; ... > y; y++) + if (IsRHSInIncrement && IsConditionGreaterThan && IsIncrement) return; + // for (; ... < y; y--) + if (IsRHSInIncrement && !IsConditionGreaterThan && !IsIncrement) return; + + // The actual warning. + const StringRef Operator = BinaryOperator::getOpcodeStr(Opcode); + SemaRef.Diag(Cond->getExprLoc(), diag::warn_loop_wrong_iteration) + << IsConditionGreaterThan << IsOrEqual << Operator << IsIncrement + << IsRHSInIncrement << Cond->getSourceRange() + << Increment->getSourceRange(); + + // One fix-it to reverse the condition operator. + const auto ReverseOpcode = BinaryOperator::reverseComparisonOp(Opcode); + const StringRef OperatorFixIt = + BinaryOperator::getOpcodeStr(ReverseOpcode); + SemaRef.Diag(Cond->getExprLoc(), + diag::note_loop_wrong_iteration_flip_condition) + << IsConditionGreaterThan << IsOrEqual + << FixItHint::CreateReplacement(Cond->getOperatorLoc(), OperatorFixIt); + + // Other fix-it to flip increment operator. + const StringRef IncrementFixIt = IsIncrement ? "--" : "++"; + SemaRef.Diag(Increment->getExprLoc(), + diag::note_loop_wrong_iteration_flip_step) + << IsIncrement + << FixItHint::CreateReplacement(Increment->getOperatorLoc(), + IncrementFixIt); + } } // end namespace @@ -1791,6 +1878,7 @@ CheckForLoopConditionalStatement(*this, Second.get().second, third.get(), Body); CheckForRedundantIteration(*this, third.get(), Body); + CheckWrongIterationDirection(*this, Second.get().second, third.get()); if (Second.get().second && !Diags.isIgnored(diag::warn_comma_operator, Index: clang/test/SemaCXX/warn-loop-analysis.cpp =================================================================== --- clang/test/SemaCXX/warn-loop-analysis.cpp +++ clang/test/SemaCXX/warn-loop-analysis.cpp @@ -295,7 +295,69 @@ for (auto[i, j, k] = arr; a < b; ++a) { } for (auto [i, j, k] = arr; i < a;) { } - for (auto[i, j, k] = arr; i < a; ++a) { } + for (auto[i, j, k] = arr; i < a; --a) { } for (auto[i, j, k] = arr; i < a; ++i) { } for (auto[i, j, k] = arr; i < a; ++arr[0]) { } }; + +void test11(int a, int b, unsigned c, unsigned d) { + // Wrong increment + for (; a < b; --a) {} + // expected-warning@-1 {{for-loop comparison has a less than operator (<) but the loop step decrements the left-hand side of the comparison}} + // expected-note@-2 {{flip comparison to greater than}} + // expected-note@-3 {{or use increment}} + for (; a <= b; a--) {} + // expected-warning@-1 {{for-loop comparison has a less than or equal to operator (<=) but the loop step decrements the left-hand side of the comparison}} + // expected-note@-2 {{flip comparison to greater than or equal to}} + // expected-note@-3 {{or use increment}} + for (; a < b; ++b) {} + // expected-warning@-1 {{for-loop comparison has a less than operator (<) but the loop step increments the right-hand side of the comparison}} + // expected-note@-2 {{flip comparison to greater than}} + // expected-note@-3 {{or use decrement}} + for (; a <= b; b++) {} + // expected-warning@-1 {{for-loop comparison has a less than or equal to operator (<=) but the loop step increments the right-hand side of the comparison}} + // expected-note@-2 {{flip comparison to greater than or equal to}} + // expected-note@-3 {{or use decrement}} + for (; a > b; ++a) {} + // expected-warning@-1 {{for-loop comparison has a greater than operator (>) but the loop step increments the left-hand side of the comparison}} + // expected-note@-2 {{flip comparison to less than}} + // expected-note@-3 {{or use decrement}} + for (; a >= b; a++) {} + // expected-warning@-1 {{for-loop comparison has a greater than or equal to operator (>=) but the loop step increments the left-hand side of the comparison}} + // expected-note@-2 {{flip comparison to less than or equal to}} + // expected-note@-3 {{or use decrement}} + for (; a > b; --b) {} + // expected-warning@-1 {{for-loop comparison has a greater than operator (>) but the loop step decrements the right-hand side of the comparison}} + // expected-note@-2 {{flip comparison to less than}} + // expected-note@-3 {{or use increment}} + for (; a >= b; b--) {} + // expected-warning@-1 {{for-loop comparison has a greater than or equal to operator (>=) but the loop step decrements the right-hand side of the comparison}} + // expected-note@-2 {{flip comparison to less than or equal to}} + // expected-note@-3 {{or use increment}} + + // Correct + for (; a < b; ++a) {} + for (; a <= b; a++) {} + for (; a < b; --b) {} + for (; a <= b; b--) {} + for (; a > b; --a) {} + for (; a >= b; a--) {} + for (; a > b; ++b) {} + for (; a >= b; b++) {} + + // Unsigned underflow and overlow are well-defined. + for (; c < d; ++c) {} + for (; c < d; --c) {} + for (; c < d; d++) {} + for (; c < d; d--) {} + + class vector { + public: + unsigned size() { return 1; }; + }; + + vector v; + for (unsigned i = v.size() - 1; i < v.size(); --i) { + // Intentionally using an underflow to allow i to equal 0. + } +}