On Sep 20, 2012, at 12:44 PM, jahanian wrote: > 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.
Right. The problem is that GetExprRange is designed around the needs of -Wconversion, where being conservative means returning a tighter range than you might otherwise: for example, -Wconversion pretends that arithmetic is generally closed on the input types, because otherwise we'd find ourselves complaining about (short = short + 1). That's not the right heuristic here, because here being conservative means returning a *broader* range. It's not insurmountable, but it wasn't worth getting right in the first iteration. John.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
