mgrabovsky created this revision. mgrabovsky added a subscriber: cfe-commits.
This change adds a Sema diagnostic for comparisons like `a < b < c`, which usually do not behave as intended by the author. Fixes bug 20082. http://reviews.llvm.org/D13643 Files: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaChecking.cpp test/Sema/bool-compare.c Index: test/Sema/bool-compare.c =================================================================== --- test/Sema/bool-compare.c +++ test/Sema/bool-compare.c @@ -85,7 +85,9 @@ if ((a<y) != -1) {}// expected-warning {{comparison of constant -1 with boolean expression is always true}} if ((a<y) == z) {} // no warning - if (a>y<z) {} // no warning + if (a>y<z) {} // expected-warning {{ternary comparisons do not work as expected}} \ + // expected-note {{to achieve the expected behavior, turn this expression into a conjunction of two comparisons}} \ + // expected-note {{place parentheses around either of the comparisons to silence this warning}} if ((a<y) > z) {} // no warning if((a<y)>(z<y)) {} // no warning if((a<y)==(z<y)){} // no warning @@ -144,10 +146,12 @@ if (z !=(a<y)) {} // no warning if (-1 !=(a<y)) {} // expected-warning {{comparison of constant -1 with boolean expression is always true}} - if (z ==(a<y)) {} // no warning - if (z<a>y) {} // no warning - if (z > (a<y)) {} // no warning - if((z<y)>(a<y)) {} // no warning + if (z ==(a<y)) {} // no warning + if (z<a>y) {} // expected-warning {{ternary comparisons do not work as expected}} \ + // expected-note {{to achieve the expected behavior, turn this expression into a conjunction of two comparisons}} \ + // expected-note {{place parentheses around either of the comparisons to silence this warning}} + if (z > (a<y)) {} // no warning + if((z<y)>(a<y)) {} // no warning if((z<y)==(a<y)){} // no warning if((z<y)!=(a<y)){} // no warning if((y==z)<(z==x)){} // no warning Index: lib/Sema/SemaChecking.cpp =================================================================== --- lib/Sema/SemaChecking.cpp +++ lib/Sema/SemaChecking.cpp @@ -6693,6 +6693,22 @@ << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange()); } +/// Diagnose attempts at ternary comparison, e.g., 1 < x < 2 +static void DiagnoseTernaryComparison(Sema &S, BinaryOperator *E) { + BinaryOperator *LHS = dyn_cast<BinaryOperator>(E->getLHS()); + if (!LHS || !LHS->isComparisonOp()) + return; + + SourceLocation Loc = E->getSourceRange().getBegin(); + + S.DiagRuntimeBehavior(Loc, E, + S.PDiag(diag::warn_ternary_comparison) << E->getSourceRange()); + S.DiagRuntimeBehavior(Loc, E, + S.PDiag(diag::note_ternary_comparison_to_conjunction)); + S.DiagRuntimeBehavior(Loc, E, + S.PDiag(diag::note_ternary_comparison_silence)); +} + /// Analyze the operands of the given comparison. Implements the /// fallback case from AnalyzeComparison. static void AnalyzeImpConvsInComparison(Sema &S, BinaryOperator *E) { @@ -6720,7 +6736,9 @@ Expr *RHS = E->getRHS()->IgnoreParenImpCasts(); bool IsComparisonConstant = false; - + + DiagnoseTernaryComparison(S, E); + // Check whether an integer constant comparison results in a value // of 'true' or 'false'. if (T->isIntegralType(S.Context)) { Index: include/clang/Basic/DiagnosticSemaKinds.td =================================================================== --- include/clang/Basic/DiagnosticSemaKinds.td +++ include/clang/Basic/DiagnosticSemaKinds.td @@ -5862,6 +5862,14 @@ def note_condition_assign_silence : Note< "place parentheses around the assignment to silence this warning">; +def warn_ternary_comparison : Warning<"ternary comparisons do not work " + "as expected">, + InGroup<Parentheses>; +def note_ternary_comparison_to_conjunction : Note<"to achieve the expected behavior, " + "turn this expression into a conjunction of two comparisons">; +def note_ternary_comparison_silence : Note<"place parentheses around either " + "of the comparisons to silence this warning">; + def warn_equality_with_extra_parens : Warning<"equality comparison with " "extraneous parentheses">, InGroup<ParenthesesOnEquality>; def note_equality_comparison_to_assign : Note<
Index: test/Sema/bool-compare.c =================================================================== --- test/Sema/bool-compare.c +++ test/Sema/bool-compare.c @@ -85,7 +85,9 @@ if ((a<y) != -1) {}// expected-warning {{comparison of constant -1 with boolean expression is always true}} if ((a<y) == z) {} // no warning - if (a>y<z) {} // no warning + if (a>y<z) {} // expected-warning {{ternary comparisons do not work as expected}} \ + // expected-note {{to achieve the expected behavior, turn this expression into a conjunction of two comparisons}} \ + // expected-note {{place parentheses around either of the comparisons to silence this warning}} if ((a<y) > z) {} // no warning if((a<y)>(z<y)) {} // no warning if((a<y)==(z<y)){} // no warning @@ -144,10 +146,12 @@ if (z !=(a<y)) {} // no warning if (-1 !=(a<y)) {} // expected-warning {{comparison of constant -1 with boolean expression is always true}} - if (z ==(a<y)) {} // no warning - if (z<a>y) {} // no warning - if (z > (a<y)) {} // no warning - if((z<y)>(a<y)) {} // no warning + if (z ==(a<y)) {} // no warning + if (z<a>y) {} // expected-warning {{ternary comparisons do not work as expected}} \ + // expected-note {{to achieve the expected behavior, turn this expression into a conjunction of two comparisons}} \ + // expected-note {{place parentheses around either of the comparisons to silence this warning}} + if (z > (a<y)) {} // no warning + if((z<y)>(a<y)) {} // no warning if((z<y)==(a<y)){} // no warning if((z<y)!=(a<y)){} // no warning if((y==z)<(z==x)){} // no warning Index: lib/Sema/SemaChecking.cpp =================================================================== --- lib/Sema/SemaChecking.cpp +++ lib/Sema/SemaChecking.cpp @@ -6693,6 +6693,22 @@ << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange()); } +/// Diagnose attempts at ternary comparison, e.g., 1 < x < 2 +static void DiagnoseTernaryComparison(Sema &S, BinaryOperator *E) { + BinaryOperator *LHS = dyn_cast<BinaryOperator>(E->getLHS()); + if (!LHS || !LHS->isComparisonOp()) + return; + + SourceLocation Loc = E->getSourceRange().getBegin(); + + S.DiagRuntimeBehavior(Loc, E, + S.PDiag(diag::warn_ternary_comparison) << E->getSourceRange()); + S.DiagRuntimeBehavior(Loc, E, + S.PDiag(diag::note_ternary_comparison_to_conjunction)); + S.DiagRuntimeBehavior(Loc, E, + S.PDiag(diag::note_ternary_comparison_silence)); +} + /// Analyze the operands of the given comparison. Implements the /// fallback case from AnalyzeComparison. static void AnalyzeImpConvsInComparison(Sema &S, BinaryOperator *E) { @@ -6720,7 +6736,9 @@ Expr *RHS = E->getRHS()->IgnoreParenImpCasts(); bool IsComparisonConstant = false; - + + DiagnoseTernaryComparison(S, E); + // Check whether an integer constant comparison results in a value // of 'true' or 'false'. if (T->isIntegralType(S.Context)) { Index: include/clang/Basic/DiagnosticSemaKinds.td =================================================================== --- include/clang/Basic/DiagnosticSemaKinds.td +++ include/clang/Basic/DiagnosticSemaKinds.td @@ -5862,6 +5862,14 @@ def note_condition_assign_silence : Note< "place parentheses around the assignment to silence this warning">; +def warn_ternary_comparison : Warning<"ternary comparisons do not work " + "as expected">, + InGroup<Parentheses>; +def note_ternary_comparison_to_conjunction : Note<"to achieve the expected behavior, " + "turn this expression into a conjunction of two comparisons">; +def note_ternary_comparison_silence : Note<"place parentheses around either " + "of the comparisons to silence this warning">; + def warn_equality_with_extra_parens : Warning<"equality comparison with " "extraneous parentheses">, InGroup<ParenthesesOnEquality>; def note_equality_comparison_to_assign : Note<
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits