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

Roman Lebedev via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Dec 21 13:14:41 PST 2017

lebedev.ri created this revision.
lebedev.ri added reviewers: aaron.ballman, rsmith, smeenai, rjmccall, rnk, mclow.lists.
lebedev.ri added a project: clang.

The diagnostic was mostly introduced in https://reviews.llvm.org/D38101 by me, as a reaction to wasting a lot of time, see mail <https://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20171009/206427.html>.
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, https://reviews.llvm.org/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 https://reviews.llvm.org/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:

- disable `warn_tautological_constant_compare` by default, make sure it is not part of `-Wall`

  FIXME: it appears `warn_unsigned_enum_always_true_comparison` also got disabled by default, which is not intentional. Do we want that?
- Add `-Wtautological-constant-compare` into `-Wextra`.

  rC Clang



Index: test/SemaCXX/compare.cpp
--- test/SemaCXX/compare.cpp
+++ 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-compare -std=c++11 %s
 int test0(long a, unsigned long b) {
   enum EnumA {A};
Index: test/Sema/tautological-constant-enum-compare.c
--- test/Sema/tautological-constant-enum-compare.c
+++ 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-compare -verify %s
+// RUN: %clang_cc1 -triple=x86_64-pc-win32 -fsyntax-only -DSIGNED -Wtautological-constant-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
Index: test/Sema/tautological-constant-compare.c
--- test/Sema/tautological-constant-compare.c
+++ test/Sema/tautological-constant-compare.c
@@ -1,7 +1,11 @@
-// 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-compare -DTEST -verify %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 -Wall -verify %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wtautological-constant-compare -DTEST -verify -x c++ %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 -x c++ %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify -x c++ %s
 int value(void);
@@ -498,7 +502,7 @@
     return 0;
 #if __SIZEOF_INT128__
-  __int128 i128;
+  __int128 i128 = value();
   if (i128 == -1) // used to crash
       return 0;
@@ -509,7 +513,7 @@
-  enum E e;
+  enum E e = (enum E)value();
   if (e == yes)
       return 0;
Index: include/clang/Basic/DiagnosticSemaKinds.td
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -5947,7 +5947,7 @@
 def warn_tautological_constant_compare : Warning<
   "result of comparison %select{%3|%1}0 %2 "
   "%select{%1|%3}0 is always %4">,
-  InGroup<TautologicalConstantCompare>;
+  InGroup<TautologicalConstantCompare>, DefaultIgnore;
 def warn_mixed_sign_comparison : Warning<
   "comparison of integers of different signs: %0 and %1">,
Index: include/clang/Basic/DiagnosticGroups.td
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -715,6 +715,7 @@
 def Move : DiagGroup<"move", [PessimizingMove, RedundantMove, SelfMove]>;
 def Extra : DiagGroup<"extra", [
+    TautologicalConstantCompare,

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D41512.127933.patch
Type: text/x-patch
Size: 4134 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171221/7a4e387a/attachment.bin>

More information about the cfe-commits mailing list