On Sep 18, 2012, at 6:13 PM, Richard Smith <[email protected]> wrote:
> On Tue, Sep 18, 2012 at 10:37 AM, Fariborz Jahanian <[email protected]> > wrote: > Author: fjahanian > Date: Tue Sep 18 12:37:21 2012 > New Revision: 164143 > > URL: http://llvm.org/viewvc/llvm-project?rev=164143&view=rev > Log: > c: warn when an integer value comparison with an > integral expression have the obvious result. > Patch reviewed by John McCall off line. > // rdar://12202422 > > Added: > cfe/trunk/test/Sema/outof-range-constant-compare.c > Modified: > cfe/trunk/include/clang/Basic/DiagnosticGroups.td > cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td > cfe/trunk/lib/Sema/SemaChecking.cpp > cfe/trunk/test/Analysis/additive-folding.cpp > cfe/trunk/test/Sema/compare.c > cfe/trunk/test/SemaCXX/compare.cpp > cfe/trunk/test/SemaCXX/for-range-examples.cpp > cfe/trunk/test/SemaCXX/warn-enum-compare.cpp > > Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=164143&r1=164142&r2=164143&view=diff > ============================================================================== > --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original) > +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Tue Sep 18 12:37:21 2012 > @@ -196,6 +196,7 @@ > def StringPlusInt : DiagGroup<"string-plus-int">; > def StrncatSize : DiagGroup<"strncat-size">; > def TautologicalCompare : DiagGroup<"tautological-compare">; > +def TautologicalOutofRangeCompare : > DiagGroup<"tautological-constant-out-of-range-compare">; > > Have you considered making this be a subgroup of -Wtautological-compare? > > def HeaderHygiene : DiagGroup<"header-hygiene">; > def DuplicateDeclSpecifier : DiagGroup<"duplicate-decl-specifier">; > > > Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=164143&r1=164142&r2=164143&view=diff > ============================================================================== > --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original) > +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Tue Sep 18 12:37:21 > 2012 > @@ -4068,6 +4068,9 @@ > def warn_lunsigned_always_true_comparison : Warning< > "comparison of unsigned%select{| enum}2 expression %0 is always %1">, > InGroup<TautologicalCompare>; > +def warn_outof_range_compare : Warning< > + "comparison of literal %0 with expression of type %1 is always " > + "%select{false|true}2">, InGroup<TautologicalOutofRangeCompare>; > > Should this be warn_out_of_range_compare, and TautologicalOutOfRangeCompare? > > Also, the constant operand isn't always a literal: > > <stdin>:1:50: warning: comparison of literal 10000 with expression of type > 'unsigned char' is always false [-Wtautological-constant-out-of-range-compare] > const int n = 10000; unsigned char c; bool b = n == c; > ~ ^ ~ > > Perhaps 'comparison of constant 10000 with expression of type ...'. > > def warn_runsigned_always_true_comparison : Warning< > "comparison of %0 unsigned%select{| enum}2 expression is always %1">, > InGroup<TautologicalCompare>; > > Modified: cfe/trunk/lib/Sema/SemaChecking.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=164143&r1=164142&r2=164143&view=diff > ============================================================================== > --- cfe/trunk/lib/Sema/SemaChecking.cpp (original) > +++ cfe/trunk/lib/Sema/SemaChecking.cpp Tue Sep 18 12:37:21 2012 > @@ -4301,6 +4301,45 @@ > } > } > > +static void DiagnoseOutOfRangeComparison(Sema &S, BinaryOperator *E, > + Expr *lit, Expr *other, > + llvm::APSInt Value, > + bool rhsLiteral) { > > Please capitalize all of these. > > + BinaryOperatorKind op = E->getOpcode(); > + QualType OtherT = other->getType(); > + const Type *OtherPtrT = S.Context.getCanonicalType(OtherT).getTypePtr(); > + const Type *LitPtrT = > S.Context.getCanonicalType(lit->getType()).getTypePtr(); > + if (OtherPtrT == LitPtrT) > + return; > > This is S.getContext().hasSameUnqualifiedType(OtherT, Lit->getType()). > > + assert((OtherT->isIntegerType() && LitPtrT->isIntegerType()) > + && "comparison with non-integer type"); > + IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT); > > Have you considered using GetExprRange on the non-constant expression, in > order to catch more cases of tautological comparisons (bitwise operations on > one of the operands, and the like). Doing this would cause false positive in ((signed char)a - 5) < (-0x81LL). We have taken a more conservative approach overall, to catch simple cases for now. > > + IntRange LitRange = GetExprRange(S.Context, lit); > > This is recomputing the constant Value that the caller just passed in. > > + if (OtherRange.Width >= LitRange.Width) > + return; > > Do you not need to check signedness here? (signed char)N == 200 isn't caught > by this check. We are aiming for catching the cases where sizes mismatch. I added a FIXME for future work. Rest of your suggestions are in r164316. Thanks for the review. - Fariborz
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
