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

[lebedev.ri]

Summary

The diagnostic was mostly introduced in D38101 by me, as a reaction to wasting a lot of time, see 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-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.

[dim]

Yes, please. Rather sooner than later, the warning itself can be fixed post-6.0.0.

[aaron.ballman]

LGTM

[dim]

Actually, having thought about it a little more, if the warning is "rather broken", or even "completely broken", depending on one's point of view, then maybe it is better not have it under -Wextra either? E.g. somebody has to ask for the warning specifically, using -Wtautological-constant-compare, or use -Weverything?

I ask this, because in FreeBSD we have traditionally been using -W, which is (again, historically) an alias for -Wextra. We now still have to explicitly use -Wno-tautological-constant-compare everywhere. :-(

[lebedev.ri]

In D41512#963743, @dim wrote:

Actually, having thought about it a little more, if the warning is "rather broken", or even "completely broken", depending on one's point of view, then maybe it is better not have it under -Wextra either?

In it's current form, there has been zero false-positives. All the complaints
are because it is finding an actual issues in the people's code, and people
don't feel like fixing the code because in some other case (data model) the
issue would not be present.

Of course, everyone is welcomed to please do prove me wrong,
and report some actual false-positives that are not data-model-dependant.

E.g. somebody has to ask for the warning specifically, using -Wtautological-constant-compare, or use -Weverything?

I ask this, because in FreeBSD we have traditionally been using -W, which is (again, historically) an alias for -Wextra. We now still have to explicitly use -Wno-tautological-constant-compare everywhere. :-(

What @aaron.ballman said in the mail. No one ever will enable it then.
And since it is not actually broken in the first place, i'm a *bit* hesitant to do that.

Though i totally understand the desire to not have bogus warnings,
and so far the clang/clang-analyzer/clang-tidy is the best at that :)

[lebedev.ri]

In D41512#963743, @dim wrote:

We now still have to explicitly use -Wno-tautological-constant-compare everywhere. :-(

OTOH, thank you for bringing this up once again. I did thought previously about
moving this specific warning into it's own flag, but never really looked into it.
But now i did, so hopefully this should be even better now :)

[rsmith]

include/clang/Basic/DiagnosticGroups.td

439

I forget, are these in-range-compare warnings new too, or just the non-unsigned-enum form? It would make sense to me for these two to be subgroups of TautologicalRangeCompare.

440

"tautological-constant-in-range-compare" would make more sense to me, as the complement of "tautological-constant-out-of-range-compare".

[lebedev.ri]

Move TautologicalUnsignedZeroCompare and TautologicalUnsignedEnumZeroCompare into TautologicalRangeCompare too, and enable them only in -Wextra.
Please advise re flag name for warn_tautological_constant_compare.

include/clang/Basic/DiagnosticGroups.td

439

I do believe all three are new: warn_unsigned_enum_always_true_comparison, warn_unsigned_always_true_comparison, warn_tautological_constant_compare.
I'll move them into TautologicalRangeCompare, and disable them too...

But now we are almost back to square one.
Disabling TautologicalRangeCompare will disable all three type-limit-checking diagnostic, and only the inner two can be enabled separately.
Is that really the intended result? I think warn_tautological_constant_compare should have it's own flag.
But i can not come up with a name for it. Please advise.

440

I did think about it.
To me "tautological-constant-in-range-compare" may sound as if the constant is somewhere in the range,
while i'd say "tautological-constant-range-compare" better highlights the fact that the constant *is* the range limit.
No?

[lebedev.ri]

• 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.
• Slightly adjust (enhance) test coverage

[lebedev.ri]

Ping. Branching is approaching rapidly. This needs to land before that.

[aaron.ballman]

include/clang/Basic/DiagnosticGroups.td

440

I don't have strong opinions on this name, but have a slight preference for "in range" because I'm left wondering how "range compare" relates to "out of range compare".

[smeenai]

include/clang/Basic/DiagnosticGroups.td

440

How about range-limit-compare or range-edge-compare or range-bound-compare or something of that nature, that explicitly captures the fact that it's the range limit?

[lebedev.ri]

@aaron.ballman, @smeenai, thank you for feedback!

include/clang/Basic/DiagnosticGroups.td

438

Any opinion on this one?

440

Ok, def TautologicalInRangeCompare : DiagGroup<"tautological-constant-in-range-compare", it is then.

[aaron.ballman]

include/clang/Basic/DiagnosticGroups.td

440

I think @smeenai may have found a name I like better: tautological-range-limit-compare. @rsmith, what do you think?

[lebedev.ri]

Renamed TautologicalRangeCompare to TautologicalInRangeCompare.

Given that previous version of this differential was already previously accepted,
and the fact that this needs to happen before the branch, which is rapidly approaching,
i'm going to proceed to committing this now.

[lebedev.ri]

Actually update the diagnostic flag :)

I'm going to hold off committing just yet, and wait for @rsmith to sign off.

[rsmith]

include/clang/Basic/DiagnosticGroups.td

438

I would *almost* prefer "tautological-min-max-compare", but I'd be worried about confusion with the std::min / std::max functions. I think this name is good enough. (Ideally, a name that clearly conveys this is about non-zero limiting values would be better, but that's hard to express.)

[thakis]

FWIW we build with -Wall -Wextra and we disable this warning since it's in our (chromium's) opinion not useful on large real-world code. So I'm not sure it should be in -Wextra. (Also, I believe clang has historically tried to keep -Wall and -Wextra pretty similar?)

include/clang/Basic/DiagnosticGroups.td

 def StaticLocalInInline : DiagGroup<"static-local-in-inline">;
 def GNUStaticFloatInit : DiagGroup<"gnu-static-float-init">;
 def StaticFloatInit : DiagGroup<"static-float-init", [GNUStaticFloatInit]>;
 def GNUStatementExpression : DiagGroup<"gnu-statement-expression">;
 def StringCompare : DiagGroup<"string-compare">;
 def StringPlusInt : DiagGroup<"string-plus-int">;
 def StringPlusChar : DiagGroup<"string-plus-char">;
 def StrncatSize : DiagGroup<"strncat-size">;
 def TautologicalUnsignedZeroCompare : DiagGroup<"tautological-unsigned-zero-compare">;
 def TautologicalUnsignedEnumZeroCompare : DiagGroup<"tautological-unsigned-enum-zero-compare">;
 def TautologicalOutOfRangeCompare : DiagGroup<"tautological-constant-out-of-range-compare">;
 def TautologicalConstantCompare : DiagGroup<"tautological-constant-compare",
                                             [TautologicalUnsignedZeroCompare,
                                              TautologicalUnsignedEnumZeroCompare,
                                              TautologicalOutOfRangeCompare]>;
 def TautologicalPointerCompare : DiagGroup<"tautological-pointer-compare">;
 def TautologicalOverlapCompare : DiagGroup<"tautological-overlap-compare">;
 def TautologicalUndefinedCompare : DiagGroup<"tautological-undefined-compare">;
 def TautologicalCompare : DiagGroup<"tautological-compare",

 
 def IntToVoidPointerCast : DiagGroup<"int-to-void-pointer-cast">;
 def IntToPointerCast : DiagGroup<"int-to-pointer-cast",
                                  [IntToVoidPointerCast]>;
 
 def Move : DiagGroup<"move", [PessimizingMove, RedundantMove, SelfMove]>;
 
 def Extra : DiagGroup<"extra", [
+    TautologicalConstantCompare,
     MissingFieldInitializers,
     IgnoredQualifiers,
     InitializerOverrides,
     SemiBeforeMethodBody,
     MissingMethodReturnType,
     SignCompare,
     UnusedParameter,
     NullPointerArithmetic

include/clang/Basic/DiagnosticSemaKinds.td

def warn_unsigned_always_true_comparison : Warning<
   InGroup<TautologicalUnsignedZeroCompare>;
 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>;
 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">,
   InGroup<SignCompare>, DefaultIgnore;
 def warn_out_of_range_compare : Warning<
   "result of comparison of %select{constant %0|true|false}1 with " 
   "%select{expression of type %2|boolean expression}3 is always %4">,
   InGroup<TautologicalOutOfRangeCompare>;

test/Sema/tautological-constant-compare.c

-// 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);
 
 #define macro(val) val
 
 #ifdef __cplusplus
 template<typename T>
 void TFunc() {
#endif
   if (32767UL <= us)
     return 0;
   if (32767UL > us)
     return 0;
   if (32767UL >= us)
     return 0;
 
 #if __SIZEOF_INT128__
-  __int128 i128;
+  __int128 i128 = value();
   if (i128 == -1) // used to crash
       return 0;
 #endif
 
 
   enum E {
   yes,
   no,
   maybe
   };
-  enum E e;
+  enum E e = (enum E)value();
 
   if (e == yes)
       return 0;
   if (e != yes)
       return 0;
   if (e < yes)
       return 0;
   if (e <= yes)

test/Sema/tautological-constant-enum-compare.c

-// 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
 
 int main() {
   enum A { A_a = 2 };
   enum A a;
 
 #ifdef SILENCE

test/SemaCXX/compare.cpp

 // 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};
   enum EnumB {B};
   enum EnumC {C = 0x10000};
   return
          // (a,b)
          (a == (unsigned long) b) +  // expected-warning {{comparison of integers of different signs}}