Combined patch for new warning work

[rtrieu]

Summary

Combined patch that contains:

https://reviews.llvm.org/D66043 - New precedence warning for mixing bitwise and/or with conditional operator
https://reviews.llvm.org/D66044 - -Wtautological-overlap-compare to work on negative numbers
https://reviews.llvm.org/D66045 - Better detection of same value operands for tautological warnings
https://reviews.llvm.org/D66046 - New warning -Wtautological-bitwise-compare to check for non-zero value bitwise-or in bool context

These are improvements to -Wparentheses, -Wtautological-compare, and -Wtautological-overlap-compare with new warning -Wtautological-bitwise-compare.

[rtrieu]

Better handling of arrays in self compares.

include/clang/AST/Expr.h

const Expr *skipRValueSubobjectAdjustments(
       SmallVectorImpl<const Expr *> &CommaLHS,
       SmallVectorImpl<SubobjectAdjustment> &Adjustments) const;
   const Expr *skipRValueSubobjectAdjustments() const {
     SmallVector<const Expr *, 8> CommaLHSs;
     SmallVector<SubobjectAdjustment, 8> Adjustments;
     return skipRValueSubobjectAdjustments(CommaLHSs, Adjustments);
   }
 
+  /// Checks that the two Expr's will refer to the same value as a comparison
+  /// operand.  The caller must ensure that the values referenced by the Expr's
+  /// are not modified between E1 and E2 or the result my be invalid.
+  static bool isSameComparisonOperand(const Expr* E1, const Expr* E2);
+
   static bool classof(const Stmt *T) {
     return T->getStmtClass() >= firstExprConstant &&
            T->getStmtClass() <= lastExprConstant;
   }
 };
 
 //===----------------------------------------------------------------------===//
 // Wrapper Expressions.

include/clang/Analysis/CFG.h

 class CFGCallback {
 public:
   CFGCallback() = default;
   virtual ~CFGCallback() = default;
 
   virtual void compareAlwaysTrue(const BinaryOperator *B, bool isAlwaysTrue) {}
   virtual void compareBitwiseEquality(const BinaryOperator *B,
                                       bool isAlwaysTrue) {}
+  virtual void compareBitwiseOr(const BinaryOperator *B) {}
 };
 
 /// Represents a source-level, intra-procedural CFG that represents the
 ///  control-flow of a Stmt.  The Stmt can represent an entire function body,
 ///  or a single expression.  A CFG will always contain one empty block that
 ///  represents the Exit point of the CFG.  A CFG will also contain a designated
 ///  Entry block.  The CFG solely represents control-flow; it consists of
 ///  CFGBlocks which are simply containers of Stmt*'s in the AST the CFG

include/clang/Basic/DiagnosticGroups.td

def TautologicalInRangeCompare : DiagGroup<"tautological-constant-in-range-compare",
                                             TautologicalUnsignedEnumZeroCompare]>;
 // For compatibility with GCC; -Wtype-limits = -Wtautological-constant-in-range-compare
 def TypeLimits : DiagGroup<"type-limits", [TautologicalInRangeCompare]>;
 def TautologicalOutOfRangeCompare : DiagGroup<"tautological-constant-out-of-range-compare">;
 def TautologicalConstantCompare : DiagGroup<"tautological-constant-compare",
                                             [TautologicalOutOfRangeCompare]>;
 def TautologicalPointerCompare : DiagGroup<"tautological-pointer-compare">;
 def TautologicalOverlapCompare : DiagGroup<"tautological-overlap-compare">;
+def TautologicalBitwiseCompare : DiagGroup<"tautological-bitwise-compare">;
 def TautologicalUndefinedCompare : DiagGroup<"tautological-undefined-compare">;
 def TautologicalObjCBoolCompare : DiagGroup<"tautological-objc-bool-compare">;
 def TautologicalCompare : DiagGroup<"tautological-compare",
                                     [TautologicalConstantCompare,
                                      TautologicalPointerCompare,
                                      TautologicalOverlapCompare,
+                                     TautologicalBitwiseCompare,
                                      TautologicalUndefinedCompare,
                                      TautologicalObjCBoolCompare]>;
 def HeaderHygiene : DiagGroup<"header-hygiene">;
 def DuplicateDeclSpecifier : DiagGroup<"duplicate-decl-specifier">;
 def CompareDistinctPointerType : DiagGroup<"compare-distinct-pointer-types">;
 def GNUUnionCast : DiagGroup<"gnu-union-cast">;
 def GNUVariableSizedTypeNotAtEnd : DiagGroup<"gnu-variable-sized-type-not-at-end">;
 def Varargs : DiagGroup<"varargs">;

include/clang/Basic/DiagnosticSemaKinds.td

 // should result in a warning, since these always evaluate to a constant.
 // Array comparisons have similar warnings
 def warn_comparison_always : Warning<
   "%select{self-|array }0comparison always evaluates to "
   "%select{a constant|true|false|'std::strong_ordering::equal'}1">,
   InGroup<TautologicalCompare>;
 def warn_comparison_bitwise_always : Warning<
   "bitwise comparison always evaluates to %select{false|true}0">,
-  InGroup<TautologicalCompare>;
+  InGroup<TautologicalBitwiseCompare>, DefaultIgnore;
+def warn_comparison_bitwise_or : Warning<
+  "bitwise or with non-zero value always evaluates to true">,
+  InGroup<TautologicalBitwiseCompare>, DefaultIgnore;
 def warn_tautological_overlap_comparison : Warning<
   "overlapping comparisons always evaluate to %select{false|true}0">,
   InGroup<TautologicalOverlapCompare>, DefaultIgnore;
 
 def warn_stringcompare : Warning<
   "result of comparison against %select{a string literal|@encode}0 is "
   "unspecified (use strncmp instead)">,
   InGroup<StringCompare>;

lib/AST/Expr.cpp

if (const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E))
     if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl()))
       if (VD->getStorageClass() == SC_Register &&
           VD->hasAttr<AsmLabelAttr>() && !VD->isLocalVarDecl())
         return true;
 
   return false;
 }
 
+bool Expr::isSameComparisonOperand(const Expr* E1, const Expr* E2) {
+  E1 = E1->IgnoreParens();
+  E2 = E2->IgnoreParens();
+
+  if (E1->getStmtClass() != E2->getStmtClass())
+    return false;
+
+  switch (E1->getStmtClass()) {
+    default:
+      return false;
+    case CXXThisExprClass:
+      return true;
+    case DeclRefExprClass: {
+      // DeclRefExpr without an ImplicitCastExpr can happen for integral
+      // template parameters.
+      const auto *DRE1 = cast<DeclRefExpr>(E1);
+      const auto *DRE2 = cast<DeclRefExpr>(E2);
+      return DRE1->isRValue() && DRE2->isRValue() &&
+             DRE1->getDecl() == DRE2->getDecl();
+    }
+    case ImplicitCastExprClass: {
+      // Peel off implicit casts.
+      while (true) {
+        const auto *ICE1 = dyn_cast<ImplicitCastExpr>(E1);
+        const auto *ICE2 = dyn_cast<ImplicitCastExpr>(E2);
+        if (!ICE1 || !ICE2)
+          return false;
+        if (ICE1->getCastKind() != ICE2->getCastKind())
+          return false;
+        E1 = ICE1->getSubExpr()->IgnoreParens();
+        E2 = ICE2->getSubExpr()->IgnoreParens();
+        // The final cast must be one of these types.
+        if (ICE1->getCastKind() == CK_LValueToRValue ||
+            ICE1->getCastKind() == CK_ArrayToPointerDecay ||
+            ICE1->getCastKind() == CK_FunctionToPointerDecay) {
+          break;
+        }
+      }
+
+      const auto *DRE1 = dyn_cast<DeclRefExpr>(E1);
+      const auto *DRE2 = dyn_cast<DeclRefExpr>(E2);
+      if (DRE1 && DRE2)
+        return declaresSameEntity(DRE1->getDecl(), DRE2->getDecl());
+
+      const auto *Ivar1 = dyn_cast<ObjCIvarRefExpr>(E1);
+      const auto *Ivar2 = dyn_cast<ObjCIvarRefExpr>(E2);
+      if (Ivar1 && Ivar2) {
+        return Ivar1->isFreeIvar() && Ivar2->isFreeIvar() &&
+               declaresSameEntity(Ivar1->getDecl(), Ivar2->getDecl());
+      }
+
+      // Walk the MemberExpr chain.
+      while (isa<MemberExpr>(E1) && isa<MemberExpr>(E2)) {
+        const auto *ME1 = cast<MemberExpr>(E1);
+        const auto *ME2 = cast<MemberExpr>(E2);
+        if (!declaresSameEntity(ME1->getMemberDecl(), ME2->getMemberDecl()))
+          return false;
+        if (const auto *D = dyn_cast<VarDecl>(ME1->getMemberDecl()))
+          if (D->isStaticDataMember())
+            return true;
+        E1 = ME1->getBase()->IgnoreParens();
+        E2 = ME2->getBase()->IgnoreParens();
+      }
+
+      if (isa<CXXThisExpr>(E1) && isa<CXXThisExpr>(E2))
+        return true;
+
+      // A static member variable can end the MemberExpr chain with either
+      // a MemberExpr or a DeclRefExpr.
+      auto getAnyDecl = [](const Expr *E) -> const ValueDecl * {
+        if (auto *DRE = dyn_cast<DeclRefExpr>(E))
+          return DRE->getDecl();
+        if (auto *ME = dyn_cast<MemberExpr>(E))
+          return ME->getMemberDecl();
+        return nullptr;
+      };
+
+      const ValueDecl *VD1 = getAnyDecl(E1);
+      const ValueDecl *VD2 = getAnyDecl(E2);
+      return declaresSameEntity(VD1, VD2);
+    }
+  }
+}
+
 /// isArrow - Return true if the base expression is a pointer to vector,
 /// return false if the base expression is a vector.
 bool ExtVectorElementExpr::isArrow() const {
   return getBase()->getType()->isPointerType();
 }
 
 unsigned ExtVectorElementExpr::getNumElements() const {
   if (const VectorType *VT = getType()->getAs<VectorType>())

lib/Analysis/CFG.cpp

 
 static SourceLocation GetEndLoc(Decl *D) {
   if (VarDecl *VD = dyn_cast<VarDecl>(D))
     if (Expr *Ex = VD->getInit())
       return Ex->getSourceRange().getEnd();
   return D->getLocation();
 }
 
+/// Returns true on constant values based around a single IntegerLiteral.
+/// Allow for use of parentheses, integer casts, and negative signs.
+static bool IsIntegerLiteralConstantExpr(const Expr *E) {
+  // Allow parentheses
+  E = E->IgnoreParens();
+
+  // Allow conversions to different integer kind.
+  if (const auto *CE = dyn_cast<CastExpr>(E)) {
+    if (CE->getCastKind() != CK_IntegralCast)
+      return false;
+    E = CE->getSubExpr();
+  }
+
+  // Allow negative numbers.
+  if (const auto *UO = dyn_cast<UnaryOperator>(E)) {
+    if (UO->getOpcode() != UO_Minus)
+      return false;
+    E = UO->getSubExpr();
+  }
+
+  return isa<IntegerLiteral>(E);
+}
+
 /// Helper for tryNormalizeBinaryOperator. Attempts to extract an IntegerLiteral
-/// or EnumConstantDecl from the given Expr. If it fails, returns nullptr.
+/// constant expression or EnumConstantDecl from the given Expr. If it fails,
+/// returns nullptr.
 static const Expr *tryTransformToIntOrEnumConstant(const Expr *E) {
   E = E->IgnoreParens();
-  if (isa<IntegerLiteral>(E))
+  if (IsIntegerLiteralConstantExpr(E))
     return E;
   if (auto *DR = dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts()))
     return isa<EnumConstantDecl>(DR->getDecl()) ? DR : nullptr;
   return nullptr;
 }
 
-/// Tries to interpret a binary operator into `Decl Op Expr` form, if Expr is
-/// an integer literal or an enum constant.
+/// Tries to interpret a binary operator into `Expr Op NumExpr` form, if
+/// NumExpr is an integer literal or an enum constant.
 ///
 /// If this fails, at least one of the returned DeclRefExpr or Expr will be
 /// null.
-static std::tuple<const DeclRefExpr *, BinaryOperatorKind, const Expr *>
+static std::tuple<const Expr *, BinaryOperatorKind, const Expr *>
 tryNormalizeBinaryOperator(const BinaryOperator *B) {
   BinaryOperatorKind Op = B->getOpcode();
 
   const Expr *MaybeDecl = B->getLHS();
   const Expr *Constant = tryTransformToIntOrEnumConstant(B->getRHS());
   // Expr looked like `0 == Foo` instead of `Foo == 0`
   if (Constant == nullptr) {
     // Flip the operator
     if (Op == BO_GT)
       Op = BO_LT;
     else if (Op == BO_GE)
       Op = BO_LE;
     else if (Op == BO_LT)
       Op = BO_GT;
     else if (Op == BO_LE)
       Op = BO_GE;
 
     MaybeDecl = B->getRHS();
     Constant = tryTransformToIntOrEnumConstant(B->getLHS());
   }
 
-  auto *D = dyn_cast<DeclRefExpr>(MaybeDecl->IgnoreParenImpCasts());
-  return std::make_tuple(D, Op, Constant);
+  return std::make_tuple(MaybeDecl, Op, Constant);
 }
 
 /// For an expression `x == Foo && x == Bar`, this determines whether the
 /// `Foo` and `Bar` are either of the same enumeration type, or both integer
 /// literals.
 ///
 /// It's an error to pass this arguments that are not either IntegerLiterals
 /// or DeclRefExprs (that have decls of type EnumConstantDecl)
 static bool areExprTypesCompatible(const Expr *E1, const Expr *E2) {
   // User intent isn't clear if they're mixing int literals with enum
   // constants.
-  if (isa<IntegerLiteral>(E1) != isa<IntegerLiteral>(E2))
+  if (isa<DeclRefExpr>(E1) != isa<DeclRefExpr>(E2))
     return false;
 
   // Integer literal comparisons, regardless of literal type, are acceptable.
-  if (isa<IntegerLiteral>(E1))
+  if (!isa<DeclRefExpr>(E1))
     return true;
 
   // IntegerLiterals are handled above and only EnumConstantDecls are expected
   // beyond this point
   assert(isa<DeclRefExpr>(E1) && isa<DeclRefExpr>(E2));
   auto *Decl1 = cast<DeclRefExpr>(E1)->getDecl();
   auto *Decl2 = cast<DeclRefExpr>(E2)->getDecl();
 
TryResult checkIncorrectLogicOperator(const BinaryOperator *B) {
     const BinaryOperator *RHS =
         dyn_cast<BinaryOperator>(B->getRHS()->IgnoreParens());
     if (!LHS || !RHS)
       return {};
 
     if (!LHS->isComparisonOp() || !RHS->isComparisonOp())
       return {};
 
-    const DeclRefExpr *Decl1;
-    const Expr *Expr1;
+    const Expr *DeclExpr1;
+    const Expr *NumExpr1;
     BinaryOperatorKind BO1;
-    std::tie(Decl1, BO1, Expr1) = tryNormalizeBinaryOperator(LHS);
+    std::tie(DeclExpr1, BO1, NumExpr1) = tryNormalizeBinaryOperator(LHS);
 
-    if (!Decl1 || !Expr1)
+    if (!DeclExpr1 || !NumExpr1)
       return {};
 
-    const DeclRefExpr *Decl2;
-    const Expr *Expr2;
+    const Expr *DeclExpr2;
+    const Expr *NumExpr2;
     BinaryOperatorKind BO2;
-    std::tie(Decl2, BO2, Expr2) = tryNormalizeBinaryOperator(RHS);
+    std::tie(DeclExpr2, BO2, NumExpr2) = tryNormalizeBinaryOperator(RHS);
 
-    if (!Decl2 || !Expr2)
+    if (!DeclExpr2 || !NumExpr2)
       return {};
 
     // Check that it is the same variable on both sides.
-    if (Decl1->getDecl() != Decl2->getDecl())
+    if (!Expr::isSameComparisonOperand(DeclExpr1, DeclExpr2))
       return {};
 
     // Make sure the user's intent is clear (e.g. they're comparing against two
     // int literals, or two things from the same enum)
-    if (!areExprTypesCompatible(Expr1, Expr2))
+    if (!areExprTypesCompatible(NumExpr1, NumExpr2))
       return {};
 
     Expr::EvalResult L1Result, L2Result;
-    if (!Expr1->EvaluateAsInt(L1Result, *Context) ||
-        !Expr2->EvaluateAsInt(L2Result, *Context))
+    if (!NumExpr1->EvaluateAsInt(L1Result, *Context) ||
+        !NumExpr2->EvaluateAsInt(L2Result, *Context))
       return {};
 
     llvm::APSInt L1 = L1Result.Val.getInt();
     llvm::APSInt L2 = L2Result.Val.getInt();
 
     // Can't compare signed with unsigned or with different bit width.
     if (L1.isSigned() != L2.isSigned() || L1.getBitWidth() != L2.getBitWidth())
       return {};
TryResult checkIncorrectLogicOperator(const BinaryOperator *B) {
 
     // Check whether expression is always true/false by evaluating the following
     // * variable x is less than the smallest literal.
     // * variable x is equal to the smallest literal.
     // * Variable x is between smallest and largest literal.
     // * Variable x is equal to the largest literal.
     // * Variable x is greater than largest literal.
     bool AlwaysTrue = true, AlwaysFalse = true;
+    // Track value of both subexpression.  If either side is always
+    // true/false, another warning should have already been emitted.
+    bool LHSAlwaysTrue = true, LHSAlwaysFalse = true;
+    bool RHSAlwaysTrue = true, RHSAlwaysFalse = true;
     for (const llvm::APSInt &Value : Values) {
       TryResult Res1, Res2;
       Res1 = analyzeLogicOperatorCondition(BO1, Value, L1);
       Res2 = analyzeLogicOperatorCondition(BO2, Value, L2);
 
       if (!Res1.isKnown() || !Res2.isKnown())
         return {};
 
       if (B->getOpcode() == BO_LAnd) {
         AlwaysTrue &= (Res1.isTrue() && Res2.isTrue());
         AlwaysFalse &= !(Res1.isTrue() && Res2.isTrue());
       } else {
         AlwaysTrue &= (Res1.isTrue() || Res2.isTrue());
         AlwaysFalse &= !(Res1.isTrue() || Res2.isTrue());
       }
+
+      LHSAlwaysTrue &= Res1.isTrue();
+      LHSAlwaysFalse &= Res1.isFalse();
+      RHSAlwaysTrue &= Res2.isTrue();
+      RHSAlwaysFalse &= Res2.isFalse();
     }
 
     if (AlwaysTrue || AlwaysFalse) {
-      if (BuildOpts.Observer)
+      if (!LHSAlwaysTrue && !LHSAlwaysFalse && !RHSAlwaysTrue &&
+          !RHSAlwaysFalse && BuildOpts.Observer)
         BuildOpts.Observer->compareAlwaysTrue(B, AlwaysTrue);
       return TryResult(AlwaysTrue);
     }
     return {};
   }
 
+  /// A bitwise-or with a non-zero constant always evaluates to true.
+  TryResult checkIncorrectBitwiseOrOperator(const BinaryOperator *B) {
+    const Expr *LHSConstant =
+        tryTransformToIntOrEnumConstant(B->getLHS()->IgnoreParenImpCasts());
+    const Expr *RHSConstant =
+        tryTransformToIntOrEnumConstant(B->getRHS()->IgnoreParenImpCasts());
+
+    if ((LHSConstant && RHSConstant) || (!LHSConstant && !RHSConstant))
+      return {};
+
+    const Expr *Constant = LHSConstant ? LHSConstant : RHSConstant;
+
+    Expr::EvalResult Result;
+    if (!Constant->EvaluateAsInt(Result, *Context))
+      return {};
+
+    if (Result.Val.getInt() == 0)
+      return {};
+
+    if (BuildOpts.Observer)
+      BuildOpts.Observer->compareBitwiseOr(B);
+
+    return TryResult(true);
+  }
+
   /// Try and evaluate an expression to an integer constant.
   bool tryEvaluate(Expr *S, Expr::EvalResult &outResult) {
     if (!BuildOpts.PruneTriviallyFalseEdges)
       return false;
     return !S->isTypeDependent() &&
            !S->isValueDependent() &&
            S->EvaluateAsRValue(outResult, *Context);
   }
 
   /// tryEvaluateBool - Try and evaluate the Stmt and return 0 or 1
   /// if we can evaluate to a known value, otherwise return -1.
   TryResult tryEvaluateBool(Expr *S) {
     if (!BuildOpts.PruneTriviallyFalseEdges ||
         S->isTypeDependent() || S->isValueDependent())
       return {};
 
     if (BinaryOperator *Bop = dyn_cast<BinaryOperator>(S)) {
-      if (Bop->isLogicalOp()) {
+      if (Bop->isLogicalOp() || Bop->isEqualityOp()) {
         // Check the cache first.
         CachedBoolEvalsTy::iterator I = CachedBoolEvals.find(S);
         if (I != CachedBoolEvals.end())
           return I->second; // already in map;
 
         // Retrieve result at first, or the map might be updated.
         TryResult Result = evaluateAsBooleanConditionNoCache(S);
         CachedBoolEvals[S] = Result; // update or insert
if (BinaryOperator *Bop = dyn_cast<BinaryOperator>(E)) {
       } else if (Bop->isEqualityOp()) {
           TryResult BopRes = checkIncorrectEqualityOperator(Bop);
           if (BopRes.isKnown())
             return BopRes.isTrue();
       } else if (Bop->isRelationalOp()) {
         TryResult BopRes = checkIncorrectRelationalOperator(Bop);
         if (BopRes.isKnown())
           return BopRes.isTrue();
+      } else if (Bop->getOpcode() == BO_Or) {
+        TryResult BopRes = checkIncorrectBitwiseOrOperator(Bop);
+        if (BopRes.isKnown())
+          return BopRes.isTrue();
       }
     }
 
     bool Result;
     if (E->EvaluateAsBooleanCondition(Result, *Context))
       return Result;
 
     return {};

 
 CFGBlock *CFGBuilder::VisitUnaryOperator(UnaryOperator *U,
            AddStmtChoice asc) {
   if (asc.alwaysAdd(*this, U)) {
     autoCreateBlock();
     appendStmt(Block, U);
   }
 
+  if (U->getOpcode() == UO_LNot)
+    tryEvaluateBool(U->getSubExpr()->IgnoreParens());
+
   return Visit(U->getSubExpr(), AddStmtChoice());
 }
 
 CFGBlock *CFGBuilder::VisitLogicalOperator(BinaryOperator *B) {
   CFGBlock *ConfluenceBlock = Block ? Block : createBlock();
   appendStmt(ConfluenceBlock, B);
 
   if (badCFG)
if (B->isAssignmentOp()) {
     return Visit(B->getRHS());
   }
 
   if (asc.alwaysAdd(*this, B)) {
     autoCreateBlock();
     appendStmt(Block, B);
   }
 
+  if (B->isEqualityOp() || B->isRelationalOp())
+    tryEvaluateBool(B);
+
   CFGBlock *RBlock = Visit(B->getRHS());
   CFGBlock *LBlock = Visit(B->getLHS());
   // If visiting RHS causes us to finish 'Block', e.g. the RHS is a StmtExpr
   // containing a DoStmt, and the LHS doesn't create a new block, then we should
   // return RBlock.  Otherwise we'll incorrectly return NULL.
   return (LBlock ? LBlock : RBlock);
 }
 

 }
 
 CFGBlock *CFGBuilder::VisitImplicitCastExpr(ImplicitCastExpr *E,
                                             AddStmtChoice asc) {
   if (asc.alwaysAdd(*this, E)) {
     autoCreateBlock();
     appendStmt(Block, E);
   }
+
+  if (E->getCastKind() == CK_IntegralToBoolean)
+    tryEvaluateBool(E->getSubExpr()->IgnoreParens());
+
   return Visit(E->getSubExpr(), AddStmtChoice());
 }
 
 CFGBlock *CFGBuilder::VisitConstantExpr(ConstantExpr *E, AddStmtChoice asc) {
   return Visit(E->getSubExpr(), AddStmtChoice());
 }
 
 CFGBlock *CFGBuilder::VisitIndirectGotoStmt(IndirectGotoStmt *I) {

lib/Analysis/ReachableCode.cpp

case Stmt::BinaryOperatorClass: {
       IncludeIntegers &= (B->isLogicalOp() || B->isComparisonOp());
       return isConfigurationValue(B->getLHS(), PP, SilenceableCondVal,
                                   IncludeIntegers) ||
              isConfigurationValue(B->getRHS(), PP, SilenceableCondVal,
                                   IncludeIntegers);
     }
     case Stmt::UnaryOperatorClass: {
       const UnaryOperator *UO = cast<UnaryOperator>(S);
-      if (UO->getOpcode() != UO_LNot)
+      if (UO->getOpcode() != UO_LNot && UO->getOpcode() != UO_Minus)
         return false;
       bool SilenceableCondValNotSet =
           SilenceableCondVal && SilenceableCondVal->getBegin().isInvalid();
       bool IsSubExprConfigValue =
           isConfigurationValue(UO->getSubExpr(), PP, SilenceableCondVal,
                                IncludeIntegers, WrappedInParens);
       // Update the silenceable condition value source range only if the range
       // was set directly by the child expression.

lib/Sema/AnalysisBasedWarnings.cpp

void compareBitwiseEquality(const BinaryOperator *B,
                               bool isAlwaysTrue) override {
     if (HasMacroID(B))
       return;
 
     SourceRange DiagRange = B->getSourceRange();
     S.Diag(B->getExprLoc(), diag::warn_comparison_bitwise_always)
         << DiagRange << isAlwaysTrue;
   }
+
+  void compareBitwiseOr(const BinaryOperator *B) override {
+    if (HasMacroID(B))
+      return;
+
+    SourceRange DiagRange = B->getSourceRange();
+    S.Diag(B->getExprLoc(), diag::warn_comparison_bitwise_or) << DiagRange;
+  }
+
+  static bool hasActiveDiagnostics(DiagnosticsEngine &Diags,
+                                   SourceLocation Loc) {
+    return !Diags.isIgnored(diag::warn_tautological_overlap_comparison, Loc) ||
+           !Diags.isIgnored(diag::warn_comparison_bitwise_or, Loc);
+  }
 };
 } // anonymous namespace
 
 //===----------------------------------------------------------------------===//
 // Check for infinite self-recursion in functions
 //===----------------------------------------------------------------------===//
 
 // Returns true if the function is called anywhere within the CFGBlock.
AC.getCFGBuildOptions()
       .setAlwaysAdd(Stmt::BlockExprClass)
       .setAlwaysAdd(Stmt::CStyleCastExprClass)
       .setAlwaysAdd(Stmt::DeclRefExprClass)
       .setAlwaysAdd(Stmt::ImplicitCastExprClass)
       .setAlwaysAdd(Stmt::UnaryOperatorClass)
       .setAlwaysAdd(Stmt::AttributedStmtClass);
   }
 
-  // Install the logical handler for -Wtautological-overlap-compare
+  // Install the logical handler.
   llvm::Optional<LogicalErrorHandler> LEH;
-  if (!Diags.isIgnored(diag::warn_tautological_overlap_comparison,
-                       D->getBeginLoc())) {
+  if (LogicalErrorHandler::hasActiveDiagnostics(Diags, D->getBeginLoc())) {
     LEH.emplace(S);
     AC.getCFGBuildOptions().Observer = &*LEH;
   }
 
   // Emit delayed diagnostics.
   if (!fscope->PossiblyUnreachableDiags.empty()) {
     bool analyzed = false;
 
AnalysisBasedWarnings::IssueWarnings(sema::AnalysisBasedWarnings::Policy P,
 
   // Check for throw out of non-throwing function.
   if (!Diags.isIgnored(diag::warn_throw_in_noexcept_func, D->getBeginLoc()))
     if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D))
       if (S.getLangOpts().CPlusPlus && isNoexcept(FD))
         checkThrowInNonThrowingFunc(S, FD, AC);
 
   // If none of the previous checks caused a CFG build, trigger one here
-  // for -Wtautological-overlap-compare
-  if (!Diags.isIgnored(diag::warn_tautological_overlap_comparison,
-                       D->getBeginLoc())) {
+  // for the logical error handler.
+  if (LogicalErrorHandler::hasActiveDiagnostics(Diags, D->getBeginLoc())) {
     AC.getCFG();
   }
 
   // Collect statistics about the CFG if it was built.
   if (S.CollectStats && AC.isCFGBuilt()) {
     ++NumFunctionsAnalyzed;
     if (CFG *cfg = AC.getCFG()) {
       // If we successfully built a CFG for this context, record some more

lib/Sema/SemaExpr.cpp

if (ParenRange.getBegin().isFileID() && ParenRange.getEnd().isFileID() &&
     // We can't display the parentheses, so just show the bare note.
     Self.Diag(Loc, Note) << ParenRange;
   }
 }
 
 static bool IsArithmeticOp(BinaryOperatorKind Opc) {
   return BinaryOperator::isAdditiveOp(Opc) ||
          BinaryOperator::isMultiplicativeOp(Opc) ||
-         BinaryOperator::isShiftOp(Opc);
+         BinaryOperator::isShiftOp(Opc) || Opc == BO_And || Opc == BO_Or;
+  // Don't use isBitwiseOp because BO_Xor is commonly used as a logical op.
 }
 
 /// IsArithmeticBinaryExpr - Returns true if E is an arithmetic binary
 /// expression, either using a built-in or overloaded operator,
 /// and sets *OpCode to the opcode and *RHSExprs to the right-hand side
 /// expression.
 static bool IsArithmeticBinaryExpr(Expr *E, BinaryOperatorKind *Opcode,
                                    Expr **RHSExprs) {
static void diagnoseLogicalNotOnLHSofCheck(Sema &S, ExprResult &LHS,
   SecondClose = S.getLocForEndOfToken(SecondClose);
   if (SecondClose.isInvalid())
     SecondOpen = SourceLocation();
   S.Diag(UO->getOperatorLoc(), diag::note_logical_not_silence_with_parens)
       << FixItHint::CreateInsertion(SecondOpen, "(")
       << FixItHint::CreateInsertion(SecondClose, ")");
 }
 
-// Get the decl for a simple expression: a reference to a variable,
-// an implicit C++ field reference, or an implicit ObjC ivar reference.
-static ValueDecl *getCompareDecl(Expr *E) {
-  if (DeclRefExpr *DR = dyn_cast<DeclRefExpr>(E))
-    return DR->getDecl();
-  if (ObjCIvarRefExpr *Ivar = dyn_cast<ObjCIvarRefExpr>(E)) {
-    if (Ivar->isFreeIvar())
-      return Ivar->getDecl();
-  }
-  if (MemberExpr *Mem = dyn_cast<MemberExpr>(E)) {
+// Returns true if E refers to a non-weak array.
+static bool checkForArray(const Expr *E) {
+  const ValueDecl *D = nullptr;
+  if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(E)) {
+    D = DR->getDecl();
+  } else if (const MemberExpr *Mem = dyn_cast<MemberExpr>(E)) {
     if (Mem->isImplicitAccess())
-      return Mem->getMemberDecl();
+      D = Mem->getMemberDecl();
   }
-  return nullptr;
+  if (!D)
+    return false;
+  return D->getType()->isArrayType() && !D->isWeak();
 }
 
 /// Diagnose some forms of syntactically-obvious tautological comparison.
 static void diagnoseTautologicalComparison(Sema &S, SourceLocation Loc,
                                            Expr *LHS, Expr *RHS,
                                            BinaryOperatorKind Opc) {
   Expr *LHSStripped = LHS->IgnoreParenImpCasts();
   Expr *RHSStripped = RHS->IgnoreParenImpCasts();
static void diagnoseTautologicalComparison(Sema &S, SourceLocation Loc,
   // often indicate logic errors in the program.
   //
   // NOTE: Don't warn about comparison expressions resulting from macro
   // expansion. Also don't warn about comparisons which are only self
   // comparisons within a template instantiation. The warnings should catch
   // obvious cases in the definition of the template anyways. The idea is to
   // warn when the typed comparison operator will always evaluate to the same
   // result.
-  ValueDecl *DL = getCompareDecl(LHSStripped);
-  ValueDecl *DR = getCompareDecl(RHSStripped);
 
   // Used for indexing into %select in warn_comparison_always
   enum {
     AlwaysConstant,
     AlwaysTrue,
     AlwaysFalse,
     AlwaysEqual, // std::strong_ordering::equal from operator<=>
   };
-  if (DL && DR && declaresSameEntity(DL, DR)) {
+
+  if (Expr::isSameComparisonOperand(LHS, RHS)) {
     unsigned Result;
     switch (Opc) {
     case BO_EQ: case BO_LE: case BO_GE:
       Result = AlwaysTrue;
       break;
     case BO_NE: case BO_LT: case BO_GT:
       Result = AlwaysFalse;
       break;
     case BO_Cmp:
       Result = AlwaysEqual;
       break;
     default:
       Result = AlwaysConstant;
       break;
     }
     S.DiagRuntimeBehavior(Loc, nullptr,
                           S.PDiag(diag::warn_comparison_always)
                               << 0 /*self-comparison*/
                               << Result);
-  } else if (DL && DR &&
-             DL->getType()->isArrayType() && DR->getType()->isArrayType() &&
-             !DL->isWeak() && !DR->isWeak()) {
+  } else if (checkForArray(LHSStripped) && checkForArray(RHSStripped)) {
     // What is it always going to evaluate to?
     unsigned Result;
     switch(Opc) {
     case BO_EQ: // e.g. array1 == array2
       Result = AlwaysFalse;
       break;
     case BO_NE: // e.g. array1 != array2
       Result = AlwaysTrue;

test/Analysis/array-struct-region.cpp

 
 struct S {
   int field;
 
 #if __cplusplus
   const struct S *getThis() const { return this; }
   const struct S *operator +() const { return this; }
 
-  bool check() const { return this == this; }
-  bool operator !() const { return this != this; }
+  // Adding zero to the this pointer silences a tautological compare warning.
+  bool check() const { return this == this + 0; }
+  bool operator !() const { return this != this + 0; }
 
   int operator *() const { return field; }
 #endif
 };
 
 #if __cplusplus
 const struct S *operator -(const struct S &s) { return &s; }
 bool operator ~(const struct S &s) { return (&s) != &s; }

test/Sema/parentheses.c

void conditional_op(int x, int y, _Bool b, void* p) {
 
   (void)(x + p ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '+'}} expected-note 2{{place parentheses}}
   (void)(p + x ? 1 : 2); // no warning
 
   (void)(p + b ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '+'}} expected-note 2{{place parentheses}}
 
   (void)(x + y > 0 ? 1 : 2); // no warning
   (void)(x + (y > 0) ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '+'}} expected-note 2{{place parentheses}}
+
+  (void)(x | b ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '|'}} expected-note 2{{place parentheses}}
+  (void)(x & b ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '&'}} expected-note 2{{place parentheses}}
+
+  (void)((x | b) ? 1 : 2);  // no warning, has parentheses
+  (void)(x | (b ? 1 : 2));  // no warning, has parentheses
+  (void)((x & b) ? 1 : 2);  // no warning, has parentheses
+  (void)(x & (b ? 1 : 2));  // no warning, has parentheses
+
+  (void)(x ^ b ? 1 : 2);  // no warning, ^ is often used as logical xor
+  (void)(x || b ? 1 : 2);  // no warning, logical operator
+  (void)(x && b ? 1 : 2);  // no warning, logical operator
 }
 
 // RUN: not %clang_cc1 -fsyntax-only -Wparentheses -Werror -fdiagnostics-show-option %s 2>&1 | FileCheck %s -check-prefix=CHECK-FLAG
 // CHECK-FLAG: error: using the result of an assignment as a condition without parentheses [-Werror,-Wparentheses]

test/Sema/warn-bitwise-compare.c

-// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-compare %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-bitwise-compare %s
 
 #define mydefine 2
 
 void f(int x) {
   if ((8 & x) == 3) {}  // expected-warning {{bitwise comparison always evaluates to false}}
   if ((x & 8) == 4) {}  // expected-warning {{bitwise comparison always evaluates to false}}
   if ((x & 8) != 4) {}  // expected-warning {{bitwise comparison always evaluates to true}}
   if ((2 & x) != 4) {}  // expected-warning {{bitwise comparison always evaluates to true}}
   if ((x | 4) == 3) {}  // expected-warning {{bitwise comparison always evaluates to false}}
   if ((x | 3) != 4) {}  // expected-warning {{bitwise comparison always evaluates to true}}
   if ((5 | x) != 3) {}  // expected-warning {{bitwise comparison always evaluates to true}}
   if ((x & 0x15) == 0x13) {} // expected-warning {{bitwise comparison always evaluates to false}}
   if ((0x23 | x) == 0x155){} // expected-warning {{bitwise comparison always evaluates to false}}
 
+  if (!!((8 & x) == 3)) {}  // expected-warning {{bitwise comparison always evaluates to false}}
+  int y = ((8 & x) == 3) ? 1 : 2;  // expected-warning {{bitwise comparison always evaluates to false}}
+
   if ((x & 8) == 8) {}
   if ((x & 8) != 8) {}
   if ((x | 4) == 4) {}
   if ((x | 4) != 4) {}
 
   if ((x & 9) == 8) {}
   if ((x & 9) != 8) {}
   if ((x | 4) == 5) {}
   if ((x | 4) != 5) {}
 
   if ((x & mydefine) == 8) {}
   if ((x | mydefine) == 4) {}
 }
+
+void g(int x) {
+  if (x | 5) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
+  if (5 | x) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
+  if (!((x | 5))) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
+}

test/Sema/warn-overlap.c

int returns(int x) {
   return x != 1 || x != 3;
   // expected-warning@-1{{overlapping comparisons always evaluate to true}}
   return x == 1 && x == 2;
   // expected-warning@-1{{overlapping comparisons always evaluate to false}}
 
   return x < 1 || x != 0;
   // expected-warning@-1{{overlapping comparisons always evaluate to true}}
 }
+
+int integer_conversion(unsigned x, int y) {
+  return x > 4 || x < 10;
+  // expected-warning@-1{{overlapping comparisons always evaluate to true}}
+  return y > 4u || y < 10u;
+  // expected-warning@-1{{overlapping comparisons always evaluate to true}}
+}
+
+int negative_compare(int x) {
+  return x > -1 || x < 1;
+  // expected-warning@-1{{overlapping comparisons always evaluate to true}}
+}
+
+int no_warning(unsigned x) {
+  return x >= 0 || x == 1;
+  // no warning since "x >= 0" is caught by a different tautological warning.
+}
+
+struct A {
+  int x;
+  int y;
+};
+
+int struct_test(struct A a) {
+  return a.x > 5 && a.y < 1;  // no warning, different variables
+
+  return a.x > 5 && a.x < 1;
+  // expected-warning@-1{{overlapping comparisons always evaluate to false}}
+  return a.y == 1 || a.y != 1;
+  // expected-warning@-1{{overlapping comparisons always evaluate to true}}
+}

test/Sema/warn-unreachable.c

 void wrapOneInFixit(struct StructWithPointer *s) {
   if (!s->p || 1) // expected-note {{silence by adding parentheses to mark code as explicitly dead}}
     return; // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:16-[[@LINE-1]]:16}:"/* DISABLES CODE */ ("
             // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:17}:")"
   wrapOneInFixit(s); // expected-warning {{code will never be executed}}
 }
 
 void unaryOpNoFixit() {
-  if (- 1)
+  if (~ 1)
     return; // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
   unaryOpNoFixit(); // expected-warning {{code will never be executed}}
 }
 
 void unaryOpStrictFixit(struct StructWithPointer *s) {
   if (!(s->p && 0)) // expected-note {{silence by adding parentheses to mark code as explicitly dead}}
     return; // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:17-[[@LINE-1]]:17}:"/* DISABLES CODE */ ("
             // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:18-[[@LINE-2]]:18}:")"

test/SemaCXX/self-comparison.cpp

 template<typename T, typename U>
 bool g() {
   // FIXME: Ideally we'd produce a self-comparison warning on the first of these.
   return
     Y<T>::n == Y<T>::n ||
     Y<T>::n == Y<U>::n;
 }
 template bool g<int, int>(); // should not produce any warnings
+
+namespace member_tests {
+struct B {
+  int field;
+  static int static_field;
+  int test(B b) {
+    return field == field;  // expected-warning {{self-comparison always evaluates to true}}
+    return static_field == static_field;  // expected-warning {{self-comparison always evaluates to true}}
+    return static_field == b.static_field;  // expected-warning {{self-comparison always evaluates to true}}
+    return B::static_field == this->static_field;  // expected-warning {{self-comparison always evaluates to true}}
+    return this == this;  // expected-warning {{self-comparison always evaluates to true}}
+
+    return field == b.field;
+    return this->field == b.field;
+  }
+};
+
+struct S {
+  int field;
+  static int static_field;
+  int array[1];
+};
+
+struct T {
+  int field;
+  static int static_field;
+  int array[1];
+  S s;
+};
+
+int struct_test(S s1, S s2, T t) {
+  return s1.field == s1.field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s2.field == s2.field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.static_field == s2.static_field;  // expected-warning {{self-comparison always evaluates to true}}
+  return S::static_field == s1.static_field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.array == s1.array;  // expected-warning {{self-comparison always evaluates to true}}
+  return t.s.static_field == S::static_field;  // expected-warning {{self-comparison always evaluates to true}}
+
+  // no warning
+  return s1.field == s2.field;
+  return s2.array == s1.array;
+
+  return s1.static_field == t.static_field;
+};
+} // namespace member_tests

test/SemaCXX/warn-bitwise-compare.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-bitwise-compare %s
+
+void test(int x) {
+  bool b1 = (8 & x) == 3;
+  // expected-warning@-1 {{bitwise comparison always evaluates to false}}
+  bool b2 = x | 5;
+  // expected-warning@-1 {{bitwise or with non-zero value always evaluates to true}}
+  bool b3 = (x | 5);
+  // expected-warning@-1 {{bitwise or with non-zero value always evaluates to true}}
+  bool b4 = !!(x | 5);
+  // expected-warning@-1 {{bitwise or with non-zero value always evaluates to true}}
+}

test/SemaCXX/warn-unreachable.cpp

void tautological_compare(bool x, int y) {
   if (y > 5 && y < 4)   // expected-note {{silence}}
     calledFun();        // expected-warning {{will never be executed}}
 
   if (y < 10 || y > 5)  // expected-note {{silence}}
     calledFun();
   else
     calledFun();        // expected-warning {{will never be executed}}
 
+  if (y == -1 && y != -1)  // expected-note {{silence}}
+    calledFun();        // expected-warning {{will never be executed}}
+
   // TODO: Extend warning to the following code:
   if (x < -1)
     calledFun();
   if (x == -1)
     calledFun();
 
   if (x != -1)
     calledFun();
   else
     calledFun();
   if (-1 > x)
     calledFun();
   else
     calledFun();
 
-  if (y == -1 && y != -1)
-    calledFun();
 }