[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

[jahanian]

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.

>  
> +  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


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120920/5535b3e6/attachment.html>