From: Roman Lebedev <lebedev.ri@gmail.com>
Date: Wed, 3 Jan 2018 08:45:19 +0000 (+0000)
Subject: [Sema] -Wtautological-constant-compare is too good. Cripple it.
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=b5b3d436a1831ddb4fdf2e4c9c07fb60807b70e5;p=clang

[Sema] -Wtautological-constant-compare is too good. Cripple it.

Summary:
The diagnostic was mostly introduced in D38101 by me, as a reaction to wasting a lot of time, see [[ https://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20171009/206427.html | mail ]].
However, the diagnostic is pretty dumb. While it works with no false-positives,
there are some questionable cases that are diagnosed when one would argue that they should not be.

The common complaint is that it diagnoses the comparisons between an `int` and
`long` when compiling for a 32-bit target as tautological, but not when
compiling for 64-bit targets. The underlying problem is obvious: data model.
In most cases, 64-bit target is `LP64` (`int` is 32-bit, `long` and pointer are
64-bit), and the 32-bit target is `ILP32` (`int`, `long`, and pointer are 32-bit).

I.e. the common pattern is: (pseudocode)
```
#include <limits>
#include <cstdint>
int main() {
  using T1 = long;
  using T2 = int;

  T1 r;
  if (r < std::numeric_limits<T2>::min()) {}
  if (r > std::numeric_limits<T2>::max()) {}
}
```
As an example, D39149 was trying to fix this diagnostic in libc++, and it was not well-received.

This *could* be "fixed", by changing the diagnostics logic to something like
`if the types of the values being compared are different, but are of the same size, then do diagnose`,
and i even attempted to do so in D39462, but as @rjmccall rightfully commented,
that implementation is incomplete to say the least.

So to stop causing trouble, and avoid contaminating upcoming release, lets do this workaround:
* move these three diags (`warn_unsigned_always_true_comparison`, `warn_unsigned_enum_always_true_comparison`, `warn_tautological_constant_compare`) into it's own `-Wtautological-constant-in-range-compare`
* Disable them by default
* Make them part of `-Wextra`
* Additionally, give `warn_tautological_constant_compare` it's own flag `-Wtautological-type-limit-compare`.
  I'm not happy about that name, but i can't come up with anything better.

This way all three of them can be enabled/disabled either altogether, or one-by-one.

Reviewers: aaron.ballman, rsmith, smeenai, rjmccall, rnk, mclow.lists, dim

Reviewed By: aaron.ballman, rsmith, dim

Subscribers: thakis, compnerd, mehdi_amini, dim, hans, cfe-commits, rjmccall

Tags: #clang

Differential Revision: https://reviews.llvm.org/D41512

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@321691 91177308-0d34-0410-b5e6-96231b3b80d8
---

diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td
index c23183c81a..79b2766ae1 100644
--- a/include/clang/Basic/DiagnosticGroups.td
+++ b/include/clang/Basic/DiagnosticGroups.td
@@ -435,12 +435,16 @@ def StringCompare : DiagGroup<"string-compare">;
 def StringPlusInt : DiagGroup<"string-plus-int">;
 def StringPlusChar : DiagGroup<"string-plus-char">;
 def StrncatSize : DiagGroup<"strncat-size">;
+def TautologicalTypeLimitCompare : DiagGroup<"tautological-type-limit-compare">;
 def TautologicalUnsignedZeroCompare : DiagGroup<"tautological-unsigned-zero-compare">;
 def TautologicalUnsignedEnumZeroCompare : DiagGroup<"tautological-unsigned-enum-zero-compare">;
+def TautologicalInRangeCompare : DiagGroup<"tautological-constant-in-range-compare",
+                                           [TautologicalTypeLimitCompare,
+                                            TautologicalUnsignedZeroCompare,
+                                            TautologicalUnsignedEnumZeroCompare]>;
 def TautologicalOutOfRangeCompare : DiagGroup<"tautological-constant-out-of-range-compare">;
 def TautologicalConstantCompare : DiagGroup<"tautological-constant-compare",
-                                            [TautologicalUnsignedZeroCompare,
-                                             TautologicalUnsignedEnumZeroCompare,
+                                            [TautologicalInRangeCompare,
                                              TautologicalOutOfRangeCompare]>;
 def TautologicalPointerCompare : DiagGroup<"tautological-pointer-compare">;
 def TautologicalOverlapCompare : DiagGroup<"tautological-overlap-compare">;
@@ -715,6 +719,7 @@ def IntToPointerCast : DiagGroup<"int-to-pointer-cast",
 def Move : DiagGroup<"move", [PessimizingMove, RedundantMove, SelfMove]>;
 
 def Extra : DiagGroup<"extra", [
+    TautologicalInRangeCompare,
     MissingFieldInitializers,
     IgnoredQualifiers,
     InitializerOverrides,
diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index e4649d3a44..ec624a0cc1 100644
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5946,15 +5946,15 @@ def note_typecheck_assign_const : Note<
 def warn_unsigned_always_true_comparison : Warning<
   "result of comparison of %select{%3|unsigned expression}0 %2 "
   "%select{unsigned expression|%3}0 is always %4">,
-  InGroup<TautologicalUnsignedZeroCompare>;
+  InGroup<TautologicalUnsignedZeroCompare>, DefaultIgnore;
 def warn_unsigned_enum_always_true_comparison : Warning<
   "result of comparison of %select{%3|unsigned enum expression}0 %2 "
   "%select{unsigned enum expression|%3}0 is always %4">,
-  InGroup<TautologicalUnsignedEnumZeroCompare>;
+  InGroup<TautologicalUnsignedEnumZeroCompare>, DefaultIgnore;
 def warn_tautological_constant_compare : Warning<
   "result of comparison %select{%3|%1}0 %2 "
   "%select{%1|%3}0 is always %4">,
-  InGroup<TautologicalConstantCompare>;
+  InGroup<TautologicalTypeLimitCompare>, DefaultIgnore;
 
 def warn_mixed_sign_comparison : Warning<
   "comparison of integers of different signs: %0 and %1">,
diff --git a/test/Sema/compare.c b/test/Sema/compare.c
index 97586a7cc0..7cd8adab89 100644
--- a/test/Sema/compare.c
+++ b/test/Sema/compare.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin -fsyntax-only -pedantic -verify -Wsign-compare %s -Wno-unreachable-code
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -fsyntax-only -pedantic -verify -Wsign-compare -Wtautological-constant-in-range-compare %s -Wno-unreachable-code
 
 int test(char *C) { // nothing here should warn.
   return C != ((void*)0);
diff --git a/test/Sema/tautological-constant-compare.c b/test/Sema/tautological-constant-compare.c
index c48aa99444..65aa7c9abd 100644
--- a/test/Sema/tautological-constant-compare.c
+++ b/test/Sema/tautological-constant-compare.c
@@ -1,7 +1,13 @@
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -DTEST -verify %s
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wno-tautological-constant-compare -verify %s
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -DTEST -verify -x c++ %s
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wno-tautological-constant-compare -verify -x c++ %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wtautological-constant-in-range-compare -DTEST -verify %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wtautological-constant-in-range-compare -DTEST -verify -x c++ %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wtautological-type-limit-compare -DTEST -verify %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wtautological-type-limit-compare -DTEST -verify -x c++ %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wextra -Wno-sign-compare -DTEST -verify %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wextra -Wno-sign-compare -DTEST -verify -x c++ %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wall -verify %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wall -verify -x c++ %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify -x c++ %s
 
 int value(void);
 
@@ -122,32 +128,6 @@ int main()
   if (32767UL >= s)
       return 0;
 
-  if (s == 0UL)
-      return 0;
-  if (s != 0UL)
-      return 0;
-  if (s < 0UL) // expected-warning {{comparison of unsigned expression < 0 is always false}}
-      return 0;
-  if (s <= 0UL)
-      return 0;
-  if (s > 0UL)
-      return 0;
-  if (s >= 0UL) // expected-warning {{comparison of unsigned expression >= 0 is always true}}
-      return 0;
-
-  if (0UL == s)
-      return 0;
-  if (0UL != s)
-      return 0;
-  if (0UL < s)
-      return 0;
-  if (0UL <= s) // expected-warning {{comparison of 0 <= unsigned expression is always true}}
-      return 0;
-  if (0UL > s) // expected-warning {{comparison of 0 > unsigned expression is always false}}
-      return 0;
-  if (0UL >= s)
-      return 0;
-
   enum { ULONG_MAX = (2UL * (unsigned long)__LONG_MAX__ + 1UL) };
   if (s == 2UL * (unsigned long)__LONG_MAX__ + 1UL)
       return 0;
@@ -498,7 +478,7 @@ int main()
     return 0;
 
 #if __SIZEOF_INT128__
-  __int128 i128;
+  __int128 i128 = value();
   if (i128 == -1) // used to crash
       return 0;
 #endif
@@ -509,7 +489,7 @@ int main()
   no,
   maybe
   };
-  enum E e;
+  enum E e = (enum E)value();
 
   if (e == yes)
       return 0;
diff --git a/test/Sema/tautological-constant-enum-compare.c b/test/Sema/tautological-constant-enum-compare.c
index 539439b817..99481c7adb 100644
--- a/test/Sema/tautological-constant-enum-compare.c
+++ b/test/Sema/tautological-constant-enum-compare.c
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -DUNSIGNED -verify %s
-// RUN: %clang_cc1 -triple=x86_64-pc-win32 -fsyntax-only -DSIGNED -verify %s
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -DUNSIGNED -Wtautological-constant-in-range-compare -verify %s
+// RUN: %clang_cc1 -triple=x86_64-pc-win32 -fsyntax-only -DSIGNED -Wtautological-constant-in-range-compare -verify %s
 // RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -DUNSIGNED -DSILENCE -Wno-tautological-constant-compare -verify %s
 // RUN: %clang_cc1 -triple=x86_64-pc-win32 -fsyntax-only -DSIGNED -DSILENCE -Wno-tautological-constant-compare -verify %s
 
diff --git a/test/Sema/tautological-unsigned-enum-zero-compare.c b/test/Sema/tautological-unsigned-enum-zero-compare.c
index a32cfcd832..87a56aa40b 100644
--- a/test/Sema/tautological-unsigned-enum-zero-compare.c
+++ b/test/Sema/tautological-unsigned-enum-zero-compare.c
@@ -1,9 +1,10 @@
 // RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only \
+// RUN:            -Wtautological-unsigned-enum-zero-compare \
 // RUN:            -verify=unsigned,unsigned-signed %s
 // RUN: %clang_cc1 -triple=x86_64-pc-win32 -fsyntax-only \
+// RUN:            -Wtautological-unsigned-enum-zero-compare \
 // RUN:            -verify=unsigned-signed %s
 // RUN: %clang_cc1 -triple=x86_64-pc-win32 -fsyntax-only \
-// RUN:            -Wno-tautological-unsigned-enum-zero-compare \
 // RUN:            -verify=silence %s
 
 // Okay, this is where it gets complicated.
diff --git a/test/Sema/tautological-unsigned-enum-zero-compare.cpp b/test/Sema/tautological-unsigned-enum-zero-compare.cpp
index a733b6edfc..0b66c68197 100644
--- a/test/Sema/tautological-unsigned-enum-zero-compare.cpp
+++ b/test/Sema/tautological-unsigned-enum-zero-compare.cpp
@@ -1,9 +1,10 @@
 // RUN: %clang_cc1 -std=c++11 -triple=x86_64-pc-linux-gnu -fsyntax-only \
+// RUN:            -Wtautological-unsigned-enum-zero-compare \
 // RUN:            -verify=unsigned,unsigned-signed %s
 // RUN: %clang_cc1 -std=c++11 -triple=x86_64-pc-win32 -fsyntax-only \
+// RUN:            -Wtautological-unsigned-enum-zero-compare \
 // RUN:            -verify=unsigned-signed %s
 // RUN: %clang_cc1 -std=c++11 -triple=x86_64-pc-win32 -fsyntax-only \
-// RUN:            -Wno-tautological-unsigned-enum-zero-compare \
 // RUN:            -verify=silence %s
 
 // silence-no-diagnostics
diff --git a/test/Sema/tautological-unsigned-zero-compare.c b/test/Sema/tautological-unsigned-zero-compare.c
index b9ea02a731..4c9394e213 100644
--- a/test/Sema/tautological-unsigned-zero-compare.c
+++ b/test/Sema/tautological-unsigned-zero-compare.c
@@ -1,7 +1,13 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
-// RUN: %clang_cc1 -fsyntax-only -Wno-tautological-unsigned-zero-compare -verify=silence %s
-// RUN: %clang_cc1 -fsyntax-only -verify -x c++ %s
-// RUN: %clang_cc1 -fsyntax-only -Wno-tautological-unsigned-zero-compare -verify=silence -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only \
+// RUN:            -Wtautological-unsigned-zero-compare \
+// RUN:            -verify %s
+// RUN: %clang_cc1 -fsyntax-only \
+// RUN:            -verify=silence %s
+// RUN: %clang_cc1 -fsyntax-only \
+// RUN:            -Wtautological-unsigned-zero-compare \
+// RUN:            -verify -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only \
+// RUN:            -verify=silence -x c++ %s
 
 unsigned uvalue(void);
 signed int svalue(void);
@@ -32,10 +38,39 @@ int main()
   TFunc<unsigned short>();
 #endif
 
+  short s = svalue();
+
   unsigned un = uvalue();
 
   // silence-no-diagnostics
 
+  // Note: both sides are promoted to unsigned long prior to the comparison.
+  if (s == 0UL)
+      return 0;
+  if (s != 0UL)
+      return 0;
+  if (s < 0UL) // expected-warning {{comparison of unsigned expression < 0 is always false}}
+      return 0;
+  if (s <= 0UL)
+      return 0;
+  if (s > 0UL)
+      return 0;
+  if (s >= 0UL) // expected-warning {{comparison of unsigned expression >= 0 is always true}}
+      return 0;
+
+  if (0UL == s)
+      return 0;
+  if (0UL != s)
+      return 0;
+  if (0UL < s)
+      return 0;
+  if (0UL <= s) // expected-warning {{comparison of 0 <= unsigned expression is always true}}
+      return 0;
+  if (0UL > s) // expected-warning {{comparison of 0 > unsigned expression is always false}}
+      return 0;
+  if (0UL >= s)
+      return 0;
+
   if (un == 0)
       return 0;
   if (un != 0)
diff --git a/test/SemaCXX/compare.cpp b/test/SemaCXX/compare.cpp
index d852180c06..ee0fe01fd1 100644
--- a/test/SemaCXX/compare.cpp
+++ b/test/SemaCXX/compare.cpp
@@ -1,7 +1,7 @@
 // Force x86-64 because some of our heuristics are actually based
 // on integer sizes.
 
-// RUN: %clang_cc1 -triple x86_64-apple-darwin -fsyntax-only -pedantic -verify -Wsign-compare -std=c++11 %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -fsyntax-only -pedantic -verify -Wsign-compare -Wtautological-constant-in-range-compare -std=c++11 %s
 
 int test0(long a, unsigned long b) {
   enum EnumA {A};