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`. Repository: rC Clang https://reviews.llvm.org/D41512 Files: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td test/Sema/tautological-constant-compare.c test/Sema/tautological-constant-enum-compare.c test/SemaCXX/compare.cpp 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; #endif @@ -509,7 +513,7 @@ no, maybe }; - 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, MissingFieldInitializers, IgnoredQualifiers, InitializerOverrides,
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; #endif @@ -509,7 +513,7 @@ no, maybe }; - 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, MissingFieldInitializers, IgnoredQualifiers, InitializerOverrides,
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits