Oops, two pings and still managed to miss this. Richard, can you take a look as
well?
Some comments from me:
- I would check for the IntegerLiteral first, because that’s a simple isa<>
check, whereas isKnownToHaveBooleanValue has to look at the expression in more
detail.
- We tend not to include empty else cases in LLVM code, even for commenting
purposes. “else { return; }” is more okay, but in general we try to keep
nesting and braces to a minimum.
- String values can be broken up in TableGen, just like in C. Please keep the
indentation consistent and break up the line into multiple string literal
chunks.
+ if (z ==(j<y)) {} //
+ if (z<k>y) {} //
+ if (z > (l<y)) {} //
+ if((z<y)>(n<y)) {} //
+ if((z<y)==(o<y)){} //
+ if((z<y)!=(p<y)){} //
+ if((y==z)<(z==x)){} //
+ if(((z==x)<(y==z))!=(q<y)){}//
- Some funky indentation on this set of test comments. I would actually just
drop all the empty comment lines, or add "no-warning” (which doesn’t do
anything, but looks like expected-warning).
- Wow, how did we miss UO_LNot as known-boolean?
Jordan
On Oct 25, 2013, at 8:27 , Per Viberg <[email protected]> wrote:
> Hi,
>
> Haven't received any comments, thus resending a patch from last week. Is
> anyone reviewing it?. it would be highly appreciated.
>
> Best regards,
> Per
>
>
>
> .......................................................................................................................
>
> Hello,
>
> The patch adds a check that warns for relational comparison of boolean
> expressions with literals 1 and 0.
> example:
>
> bool x;
> int i,e,y;
>
> if(x > 1){}
> if(!i < 0){}
> if(1 > (e<y)){}
>
> generates warning:
> "relational comparison of boolean expression against literal value '1' or '0'
> "
>
> see test files bool-compare.c and bool-compare.cpp for more details.
>
>
> Best regards,
> Per
>
> .......................................................................................................................
> Per Viberg Senior Engineer
> Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden
> Phone: +46 (0)8 402 79 00
> Mobile: +46 (0)70 912 42 52
> E-mail: [email protected]
>
> www.evidente.se
> This e-mail, which might contain confidential information, is addressed to
> the above stated person/company. If you are not the correct addressee,
> employee or in any other way the person concerned, please notify the sender
> immediately. At the same time, please delete this e-mail and destroy any
> prints. Thank You.
Index: test/Sema/bool-compare.c
===================================================================
--- test/Sema/bool-compare.c (revision 0)
+++ test/Sema/bool-compare.c (working copy)
@@ -0,0 +1,115 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+
+void f(int x, int y, int z) {
+
+ //rev 0.2
+
+ int a,b,bb,c,d,e,f,g,h,i,j,k,l,m,n,o,p,q;
+
+ if (!a > 0) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (!b > 1) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (!bb > 2) {} //
+ if (!c > y) {} //
+
+ if (!a < 0) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (!b < 1) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (!bb < 2) {} //
+ if (!c < y) {} //
+
+ if (!a >= 0) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (!b >= 1) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (!bb >= 2) {} //
+ if (!c >= y) {} //
+
+ if (!a <= 0) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (!b <= 1) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (!bb <= 2) {} //
+ if (!c <= y) {} //
+
+ if ((a||b) > 0) { } // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if ((a||b) > 1) { } // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if ((a||b) > 4) { } //
+
+ if ((a&&b) > 0) { } // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if ((a&&b) > 1) { } // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if ((a&&b) > 4) { } //
+
+ if ((d<y) > 0) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if ((e<y) > 1) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if ((e<y) > 4) {} //
+ if ((f<y) > z) {} //
+
+ if ((g<y) == 0) {} //
+ if ((h<y) == 1) {} //
+ if ((h<y) == 2) {} //
+ if ((h<y) == z) {} //
+
+ if ((i<y) != 0) {}
+ if ((i<y) != 1) {}
+ if ((i<y) != 2) {}
+ if ((i<y) != z) {}
+
+ if ((j<y) == z) {} //
+ if (k>y<z) {} //
+ if ((l<y) > z) {} //
+ if((n<y)>(z<y)) {} //
+ if((o<y)==(z<y)) {} //
+ if((p<y)!=(z<y)) {} //
+ if((z==x)<(y==z)){} //
+ if((q<y)!=((z==x)<(y==z))){} //
+
+
+ if (0 > !a) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (1 > !b) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (2 > !bb) {} //
+ if (y > !c) {} //
+
+ if (0 < !a) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (1 < !b) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (2 < !bb) {} //
+ if (y < !c) {} //
+
+ if (0 >= !a) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (1 >= !b) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (2 >= !bb) {} //
+ if (y >= !c) {} //
+
+ if (0 <= !a) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (1 <= !b) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (2 <= !bb) {} //
+ if (y <= !c) {} //
+
+ if (0 > (a||b)) { } // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (1 > (a||b)) { } // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (4 > (a||b)) { } //
+
+ if (0 > (a&&b)) { } // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (1 > (a&&b)) { } // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (4 > (a&&b)) { } //
+
+ if (0 > (d<y)) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (1 > (e<y)) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (4 > (e<y)) {} //
+ if (z > (f<y)) {} //
+
+ if (0 == (g<y)) {} //
+ if (1 == (h<y)) {} //
+ if (2 == (h<y)) {} //
+ if (z == (h<y)) {} //
+
+ if (0 !=(i<y)) {}
+ if (1 !=(i<y)) {}
+ if (2 !=(i<y)) {}
+ if (z !=(i<y)) {}
+
+ if (z ==(j<y)) {} //
+ if (z<k>y) {} //
+ if (z > (l<y)) {} //
+ if((z<y)>(n<y)) {} //
+ if((z<y)==(o<y)) {} //
+ if((z<y)!=(p<y)) {} //
+ if((y==z)<(z==x)){} //
+ if(((z==x)<(y==z))!=(q<y)){}//
+
+}
Index: test/Sema/bool-compare.cpp
===================================================================
--- test/Sema/bool-compare.cpp (revision 0)
+++ test/Sema/bool-compare.cpp (working copy)
@@ -0,0 +1,152 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+
+void f(bool x, bool y, bool z) {
+
+ bool a,b,bb,c,d,e,f,g,h,i,j,k,l,m,n,o,p,q;
+
+ if(a<b) {} //
+ if(!a<b){} //
+ if(a<!b){} //
+
+ if(a<=b) {} //
+ if(!a<=b){} //
+ if(a<=!b){} //
+
+
+ if (!a > 0) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (!b > 1) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (!bb > 2) {} // expected-warning {{comparison of constant 2 with expression of type 'bool' is always false}}
+ if (!c > y) {} //
+
+ if (!a < 0) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (!b < 1) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (!bb < 2) {} // expected-warning {{comparison of constant 2 with expression of type 'bool' is always true}}
+ if (!c < y) {} //
+
+ if (!a >= 0) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (!b >= 1) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (!bb >= 2) {} // expected-warning {{comparison of constant 2 with expression of type 'bool' is always false}}
+ if (!c >= y) {} //
+
+ if (!a <= 0) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (!b <= 1) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (!bb <= 2) {} // expected-warning {{comparison of constant 2 with expression of type 'bool' is always true}}
+ if (!c <= y) {} //
+
+ if ((a||b) > 0) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if ((a||b) > 1) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if ((a||b) > 4) {} // expected-warning {{comparison of constant 4 with expression of type 'bool' is always false}}
+
+ if ((a&&b) > 0) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if ((a&&b) > 1) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if ((a&&b) > 4) {} // expected-warning {{comparison of constant 4 with expression of type 'bool' is always false}}
+
+ if ((d<y) > 0) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if ((e<y) > 1) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if ((e<y) > 4) {} // expected-warning {{comparison of constant 4 with expression of type 'bool' is always false}}
+ if ((f<y) > z) {} //
+
+ if ((g<y) == 0) {} //
+ if ((h<y) == 1) {} //
+ if ((h<y) == 2) {} // expected-warning {{comparison of constant 2 with expression of type 'bool' is always false}}
+ if ((h<y) == z) {} //
+
+ if ((i<y) != 0) {}
+ if ((i<y) != 1) {}
+ if ((i<y) != 2) {} // expected-warning {{comparison of constant 2 with expression of type 'bool' is always true}}
+ if ((i<y) != z) {}
+
+ if ((j<y) == z) {} //
+ if (k>y<z) {} //
+ if ((l<y) > z) {} //
+ if((n<y)>(z<y)) {} //
+ if((o<y)==(z<y)){} //
+ if((p<y)!=(z<y)){} //
+ if((z==x)<(y==z)){} //
+ if((q<y)!=((z==x)<(y==z))){} //
+
+
+ if (0 > !a) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (1 > !b) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (2 > !bb) {} // expected-warning {{comparison of constant 2 with expression of type 'bool' is always true}}
+ if (y > !c) {} //
+
+ if (0 < !a) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (1 < !b) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (2 < !bb) {} // expected-warning {{comparison of constant 2 with expression of type 'bool' is always false}}
+ if (y < !c) {} //
+
+ if (0 >= !a) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (1 >= !b) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (2 >= !bb) {} // expected-warning {{comparison of constant 2 with expression of type 'bool' is always true}}
+ if (y >= !c) {} //
+
+ if (0 <= !a) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (1 <= !b) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (2 <= !bb) {} // expected-warning {{comparison of constant 2 with expression of type 'bool' is always false}}
+ if (y <= !c) {} //
+
+ if (0 > (a||b)) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (1 > (a||b)) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (4 > (a||b)) {} // expected-warning {{comparison of constant 4 with expression of type 'bool' is always true}}
+
+ if (0 > (a&&b)) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (1 > (a&&b)) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (4 > (a&&b)) {} // expected-warning {{comparison of constant 4 with expression of type 'bool' is always true}}
+
+ if (0 > (d<y)) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (1 > (e<y)) {} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+ if (4 > (e<y)) {} // expected-warning {{comparison of constant 4 with expression of type 'bool' is always true}}
+ if (z > (f<y)) {} //
+
+ if (0 == (g<y)) {} //
+ if (1 == (h<y)) {} //
+ if (2 == (h<y)) {} // expected-warning {{comparison of constant 2 with expression of type 'bool' is always false}}
+ if (z == (h<y)) {} //
+
+ if (0 !=(i<y)) {}
+ if (1 !=(i<y)) {}
+ if (2 !=(i<y)) {} // expected-warning {{comparison of constant 2 with expression of type 'bool' is always true}}
+ if (z !=(i<y)) {}
+
+ if (z ==(j<y)) {} //
+ if (z<k>y) {} //
+ if (z > (l<y)) {} //
+ if((z<y)>(n<y)) {} //
+ if((z<y)==(o<y)){} //
+ if((z<y)!=(p<y)){} //
+ if((y==z)<(z==x)){} //
+ if(((z==x)<(y==z))!=(q<y)){}//
+
+}
+
+
+template<typename T, typename U, typename V> struct X6 {
+ U f(T t, U u, V v) {
+ // IfStmt
+ if (t > 0)
+ return u;
+ else {
+ if (t < 0)
+ return v; // expected-error{{cannot initialize return object of type}}
+ }
+ bool r;
+ if(r<0){} // expected-warning {{relational comparison of boolean expression against literal value '1' or '0'}}
+
+ if (T x = t) {
+ t = x;
+ }
+ return v; // expected-error{{cannot initialize return object of type}}
+ }
+};
+
+struct ConvertibleToInt {
+ operator int() const;
+};
+
+template struct X6<ConvertibleToInt, float, char>;
+template struct X6<bool, int, int*>; // expected-note{{instantiation}}
+
+
+
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp (revision 192967)
+++ lib/Sema/SemaExpr.cpp (working copy)
@@ -7583,6 +7583,53 @@
return 0;
}
+static void diagnoseRelationalCompare(Sema &Self, SourceLocation Loc,
+ const Expr *LHS, const Expr *RHS) {
+ const FunctionDecl *Fdec = Self.getCurFunctionDecl();
+ if (Fdec) {
+ if (Fdec->isTemplateInstantiation()) {
+ // Ignore comparison between template parameter and integer literals.
+ return;
+ }
+ }
+ bool Expr1HasBooleanValue = LHS->isKnownToHaveBooleanValue();
+ bool Expr2HasBooleanValue = RHS->isKnownToHaveBooleanValue();
+ const Expr *OtherSideExpression;
+
+ if (Expr1HasBooleanValue) {
+ if (Expr2HasBooleanValue) {
+ // No warning, since both sides are boolean expressions.
+ return;
+ } else {
+ // Possible warning if RHS is literal integer of value 1 or 0.
+ OtherSideExpression = RHS;
+ }
+ } else if (Expr2HasBooleanValue) {
+ // Possible warning if LHS is literal integer of value 1 or 0.
+ OtherSideExpression = LHS;
+ } else {
+ // No warning, since neither side is boolean expression.
+ return;
+ }
+
+ // One and only one side of the relational comparison is a boolean expression.
+ // It should warn if the other side is integer literal with value 0 or 1.
+ const IntegerLiteral *IntLit = dyn_cast<IntegerLiteral>(OtherSideExpression);
+ if (IntLit) {
+ llvm::APInt IntLitValue = IntLit->getValue();
+ if ((IntLitValue == 1) || (IntLitValue == 0)) {
+ // Warning, since one side is boolean expression and the other side is an
+ // integer literal of value 1 or 0.
+ Self.Diag(Loc, diag::warn_relational_comparison_of_booleans) << Loc;
+ } else { // No warning. It is integer literal, but value is not 0 or 1.
+ }
+ } else { // No warning. It is not integer literal.
+ }
+}
+
+
+
+
// C99 6.5.8, C++ [expr.rel]
QualType Sema::CheckCompareOperands(ExprResult &LHS, ExprResult &RHS,
SourceLocation Loc, unsigned OpaqueOpc,
@@ -7595,7 +7642,9 @@
if (LHS.get()->getType()->isVectorType() ||
RHS.get()->getType()->isVectorType())
return CheckVectorCompareOperands(LHS, RHS, Loc, IsRelational);
-
+ if (IsRelational) {
+ diagnoseRelationalCompare(*this, Loc, LHS.get(), RHS.get());
+ }
QualType LHSType = LHS.get()->getType();
QualType RHSType = RHS.get()->getType();
Index: lib/AST/Expr.cpp
===================================================================
--- lib/AST/Expr.cpp (revision 192967)
+++ lib/AST/Expr.cpp (working copy)
@@ -151,6 +151,8 @@
switch (UO->getOpcode()) {
case UO_Plus:
return UO->getSubExpr()->isKnownToHaveBooleanValue();
+ case UO_LNot:
+ return true;
default:
return false;
}
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td (revision 192967)
+++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
@@ -6151,6 +6151,12 @@
def note_ref_subobject_of_member_declared_here : Note<
"member with reference subobject declared here">;
+// A relational comparison of boolean expressions should result
+// in a warning. Integer literals of value 0 or 1 are also considered booleans.
+def warn_relational_comparison_of_booleans : Warning<
+"relational comparison of boolean expression against literal value '1' or '0'">,
+ InGroup<TautologicalCompare>;
+
// For non-floating point, expressions of the form x == x or x != x
// should result in a warning, since these always evaluate to a constant.
// Array comparisons have similar warnings
> _______________________________________________
> cfe-commits mailing list
> [email protected]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
