[cfe-commits] r164143 - in /cfe/trunk: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaChecking.cpp test/Analysis/additive-folding.cpp test/Sema/compare.c test/Sema/outof-range-constant-compare.c te

[John McCall]

On Sep 20, 2012, at 12:44 PM, jahanian wrote:
> On Sep 18, 2012, at 6:13 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> 
>> On Tue, Sep 18, 2012 at 10:37 AM, Fariborz Jahanian <fjahanian at apple.com> 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120920/d3cb56ae/attachment.html>