[ObjC] Add a -Wtautological-compare warning for BOOL

On macOS, BOOL is a typedef for signed char, but it should never hold a value
that isn't 1 or 0. Any code that expects a different value in their BOOL should
be fixed.

rdar://51954400

Differential revision: https://reviews.llvm.org/D63856

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

diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td
index 9bfea83db462bed612b323492ad0eb26de6f5107..27671df0cfd6bfab7487645b63a0f3785a52757a 100644 (file)
--- a/include/clang/Basic/DiagnosticGroups.td
+++ b/include/clang/Basic/DiagnosticGroups.td
@@ -494,11 +494,13 @@ def TautologicalConstantCompare : DiagGroup<"tautological-constant-compare",
 def TautologicalPointerCompare : DiagGroup<"tautological-pointer-compare">;
 def TautologicalOverlapCompare : DiagGroup<"tautological-overlap-compare">;
 def TautologicalUndefinedCompare : DiagGroup<"tautological-undefined-compare">;
+def TautologicalObjCBoolCompare : DiagGroup<"tautological-objc-bool-compare">;
 def TautologicalCompare : DiagGroup<"tautological-compare",
                                     [TautologicalConstantCompare,
                                      TautologicalPointerCompare,
                                      TautologicalOverlapCompare,
-                                     TautologicalUndefinedCompare]>;
+                                     TautologicalUndefinedCompare,
+                                     TautologicalObjCBoolCompare]>;
 def HeaderHygiene : DiagGroup<"header-hygiene">;
 def DuplicateDeclSpecifier : DiagGroup<"duplicate-decl-specifier">;
 def CompareDistinctPointerType : DiagGroup<"compare-distinct-pointer-types">;

diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index 5fd191a030c2f33aeabc2e8212ec21ee6fec0f54..053655201585f5d4856e56d232ac834d63775402 100644 (file)
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6026,6 +6026,10 @@ def warn_tautological_constant_compare : Warning<
   "result of comparison %select{%3|%1}0 %2 "
   "%select{%1|%3}0 is always %4">,
   InGroup<TautologicalTypeLimitCompare>, DefaultIgnore;
+def warn_tautological_compare_objc_bool : Warning<
+  "result of comparison of constant %0 with expression of type BOOL"
+  " is always %1, as the only well defined values for BOOL are YES and NO">,
+  InGroup<TautologicalObjCBoolCompare>;
 
 def warn_mixed_sign_comparison : Warning<
   "comparison of integers of different signs: %0 and %1">,

diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp
index 92951a8e3bde3c0163af34d447b4ab41ab2dce9c..702a23059681ba0eaf113c8ffcfd5d49debfc046 100644 (file)
--- a/lib/Sema/SemaChecking.cpp
+++ b/lib/Sema/SemaChecking.cpp
@@ -10188,9 +10188,16 @@ static bool IsEnumConstOrFromMacro(Sema &S, Expr *E) {
     if (isa<EnumConstantDecl>(DR->getDecl()))
       return true;
 
-  // Suppress cases where the '0' value is expanded from a macro.
-  if (E->getBeginLoc().isMacroID())
-    return true;
+  // Suppress cases where the value is expanded from a macro, unless that macro
+  // is how a language represents a boolean literal. This is the case in both C
+  // and Objective-C.
+  SourceLocation BeginLoc = E->getBeginLoc();
+  if (BeginLoc.isMacroID()) {
+    StringRef MacroName = Lexer::getImmediateMacroName(
+        BeginLoc, S.getSourceManager(), S.getLangOpts());
+    return MacroName != "YES" && MacroName != "NO" &&
+           MacroName != "true" && MacroName != "false";
+  }
 
   return false;
 }
@@ -10382,11 +10389,17 @@ static bool CheckTautologicalComparison(Sema &S, BinaryOperator *E,
     OtherT = AT->getValueType();
   IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT);
 
+  // Special case for ObjC BOOL on targets where its a typedef for a signed char
+  // (Namely, macOS).
+  bool IsObjCSignedCharBool = S.getLangOpts().ObjC &&
+                              S.NSAPIObj->isObjCBOOLType(OtherT) &&
+                              OtherT->isSpecificBuiltinType(BuiltinType::SChar);
+
   // Whether we're treating Other as being a bool because of the form of
   // expression despite it having another type (typically 'int' in C).
   bool OtherIsBooleanDespiteType =
       !OtherT->isBooleanType() && Other->isKnownToHaveBooleanValue();
-  if (OtherIsBooleanDespiteType)
+  if (OtherIsBooleanDespiteType || IsObjCSignedCharBool)
     OtherRange = IntRange::forBoolType();
 
   // Determine the promoted range of the other type and see if a comparison of
@@ -10417,10 +10430,21 @@ static bool CheckTautologicalComparison(Sema &S, BinaryOperator *E,
   // Should be enough for uint128 (39 decimal digits)
   SmallString<64> PrettySourceValue;
   llvm::raw_svector_ostream OS(PrettySourceValue);
-  if (ED)
+  if (ED) {
     OS << '\'' << *ED << "' (" << Value << ")";
-  else
+  } else if (auto *BL = dyn_cast<ObjCBoolLiteralExpr>(
+               Constant->IgnoreParenImpCasts())) {
+    OS << (BL->getValue() ? "YES" : "NO");
+  } else {
     OS << Value;
+  }
+
+  if (IsObjCSignedCharBool) {
+    S.DiagRuntimeBehavior(E->getOperatorLoc(), E,
+                          S.PDiag(diag::warn_tautological_compare_objc_bool)
+                              << OS.str() << *Result);
+    return true;
+  }
 
   // FIXME: We use a somewhat different formatting for the in-range cases and
   // cases involving boolean values for historical reasons. We should pick a

diff --git a/test/Sema/tautological-objc-bool-compare.m b/test/Sema/tautological-objc-bool-compare.m
new file mode 100644 (file)
index 0000000..525862e
--- /dev/null
+++ b/test/Sema/tautological-objc-bool-compare.m
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 %s -verify
+
+typedef signed char BOOL;
+#define YES __objc_yes
+#define NO __objc_no
+
+BOOL B;
+
+void test() {
+  int r;
+  r = B > 0;
+  r = B > 1; // expected-warning {{result of comparison of constant 1 with expression of type BOOL is always false, as the only well defined values for BOOL are YES and NO}}
+  r = B < 1;
+  r = B < 0; // expected-warning {{result of comparison of constant 0 with expression of type BOOL is always false, as the only well defined values for BOOL are YES and NO}}
+  r = B >= 0; // expected-warning {{result of comparison of constant 0 with expression of type BOOL is always true, as the only well defined values for BOOL are YES and NO}}
+  r = B <= 0;
+
+  r = B > YES; // expected-warning {{result of comparison of constant YES with expression of type BOOL is always false, as the only well defined values for BOOL are YES and NO}}
+  r = B > NO;
+  r = B < NO; // expected-warning {{result of comparison of constant NO with expression of type BOOL is always false, as the only well defined values for BOOL are YES and NO}}
+  r = B < YES;
+  r = B >= NO; // expected-warning {{result of comparison of constant NO with expression of type BOOL is always true, as the only well defined values for BOOL are YES and NO}}
+  r = B <= NO;
+}