Expand -Wlogical-not-parentheses to also fire on `!x & A`.

[thakis]

Summary

This is a misspelling of the intended !(x & A) negated bit test that happens in practice every now and then.

I ran this on Chromium and all its dependencies, and it fired 0 times -- no false or true positives, but it would've caught a bug in an in-progress change that had to be caught by a Visual Studio warning instead.

[hans]

lgtm, very nice! (with one clarifying question)

test/SemaCXX/warn-logical-not-compare.cpp

224

And this is the reason you had to put a "not" on the second run line, right?

[thakis]

Thanks! Landing…

test/SemaCXX/warn-logical-not-compare.cpp

224

Right.

[thakis]

285310

include/clang/Basic/DiagnosticSemaKinds.td

 def warn_null_in_comparison_operation : Warning<
   "comparison between NULL and non-pointer "
   "%select{(%1 and NULL)|(NULL and %1)}0">,
   InGroup<NullArithmetic>;
 def err_shift_rhs_only_vector : Error<
   "requested shift is a vector of type %0 but the first operand is not a "
   "vector (%1)">;
 
-def warn_logical_not_on_lhs_of_comparison : Warning<
-  "logical not is only applied to the left hand side of this comparison">,
+def warn_logical_not_on_lhs_of_check : Warning<
+  "logical not is only applied to the left hand side of this "
+  "%select{comparison|bitwise operator}0">,
   InGroup<LogicalNotParentheses>;
 def note_logical_not_fix : Note<
-  "add parentheses after the '!' to evaluate the comparison first">;
+  "add parentheses after the '!' to evaluate the "
+  "%select{comparison|bitwise operator}0 first">;
 def note_logical_not_silence_with_parens : Note<
   "add parentheses around left hand side expression to silence this warning">;
 
 def err_invalid_this_use : Error<
   "invalid use of 'this' outside of a non-static member function">;
 def err_this_static_member_func : Error<
   "'this' cannot be%select{| implicitly}0 used in a static member function "
   "declaration">;

include/clang/Sema/Sema.h

QualType CheckSubtractionOperands( // C99 6.5.6
     QualType* CompLHSTy = nullptr);
   QualType CheckShiftOperands( // C99 6.5.7
     ExprResult &LHS, ExprResult &RHS, SourceLocation Loc,
     BinaryOperatorKind Opc, bool IsCompAssign = false);
   QualType CheckCompareOperands( // C99 6.5.8/9
     ExprResult &LHS, ExprResult &RHS, SourceLocation Loc,
     BinaryOperatorKind Opc, bool isRelational);
   QualType CheckBitwiseOperands( // C99 6.5.[10...12]
       ExprResult &LHS, ExprResult &RHS, SourceLocation Loc,
-    bool IsCompAssign = false);
+      BinaryOperatorKind Opc);
   QualType CheckLogicalOperands( // C99 6.5.[13,14]
     ExprResult &LHS, ExprResult &RHS, SourceLocation Loc,
     BinaryOperatorKind Opc);
   // CheckAssignmentOperands is used for both simple and compound assignment.
   // For simple assignment, pass both expressions and a null converted type.
   // For compound assignment, pass both expressions and the converted type.
   QualType CheckAssignmentOperands( // C99 6.5.16.[1,2]
     Expr *LHSExpr, ExprResult &RHS, SourceLocation Loc, QualType CompoundType);

lib/Sema/SemaExpr.cpp

if (BinaryOperator::isEqualityOp(Opc) &&
 
     S.Diag(Loc, diag::note_objc_literal_comparison_isequal)
       << FixItHint::CreateInsertion(Start, Opc == BO_EQ ? "[" : "![")
       << FixItHint::CreateReplacement(OpRange, " isEqual:")
       << FixItHint::CreateInsertion(End, "]");
   }
 }
 
-static void diagnoseLogicalNotOnLHSofComparison(Sema &S, ExprResult &LHS,
-                                                ExprResult &RHS,
-                                                SourceLocation Loc,
-                                                BinaryOperatorKind Opc) {
+/// Warns on !x < y, !x & y where !(x < y), !(x & y) was probably intended.
+static void diagnoseLogicalNotOnLHSofCheck(Sema &S, ExprResult &LHS,
+                                           ExprResult &RHS, SourceLocation Loc,
   // Check that left hand side is !something.
   UnaryOperator *UO = dyn_cast<UnaryOperator>(LHS.get()->IgnoreImpCasts());
   if (!UO || UO->getOpcode() != UO_LNot) return;
 
   // Only check if the right hand side is non-bool arithmetic type.
   if (RHS.get()->isKnownToHaveBooleanValue()) return;
 
   // Make sure that the something in !something is not bool.
   Expr *SubExpr = UO->getSubExpr()->IgnoreImpCasts();
   if (SubExpr->isKnownToHaveBooleanValue()) return;
 
   // Emit warning.
-  S.Diag(UO->getOperatorLoc(), diag::warn_logical_not_on_lhs_of_comparison)
-      << Loc;
+  bool IsBitwiseOp = Opc == BO_And || Opc == BO_Or || Opc == BO_Xor;
+  S.Diag(UO->getOperatorLoc(), diag::warn_logical_not_on_lhs_of_check)
+      << Loc << IsBitwiseOp;
 
   // First note suggest !(x < y)
   SourceLocation FirstOpen = SubExpr->getLocStart();
   SourceLocation FirstClose = RHS.get()->getLocEnd();
   FirstClose = S.getLocForEndOfToken(FirstClose);
   if (FirstClose.isInvalid())
     FirstOpen = SourceLocation();
   S.Diag(UO->getOperatorLoc(), diag::note_logical_not_fix)
+      << IsBitwiseOp
       << FixItHint::CreateInsertion(FirstOpen, "(")
       << FixItHint::CreateInsertion(FirstClose, ")");
 
   // Second note suggests (!x) < y
   SourceLocation SecondOpen = LHS.get()->getLocStart();
   SourceLocation SecondClose = LHS.get()->getLocEnd();
   SecondClose = S.getLocForEndOfToken(SecondClose);
   if (SecondClose.isInvalid())
QualType Sema::CheckCompareOperands(ExprResult &LHS, ExprResult &RHS,
 
   QualType LHSType = LHS.get()->getType();
   QualType RHSType = RHS.get()->getType();
 
   Expr *LHSStripped = LHS.get()->IgnoreParenImpCasts();
   Expr *RHSStripped = RHS.get()->IgnoreParenImpCasts();
 
   checkEnumComparison(*this, Loc, LHS.get(), RHS.get());
-  diagnoseLogicalNotOnLHSofComparison(*this, LHS, RHS, Loc, Opc);
+  diagnoseLogicalNotOnLHSofCheck(*this, LHS, RHS, Loc, Opc);
 
   if (!LHSType->hasFloatingRepresentation() &&
       !(LHSType->isBlockPointerType() && IsRelational) &&
       !LHS.get()->getLocStart().isMacroID() &&
       !RHS.get()->getLocStart().isMacroID() &&
       ActiveTemplateInstantiations.empty()) {
     // For non-floating point types, check for self-comparisons of the form
     // x == x, x != x, x < x, etc.  These always evaluate to a constant, and
if (vType.isNull())
     return InvalidOperands(Loc, LHS, RHS);
   if (getLangOpts().OpenCL && getLangOpts().OpenCLVersion < 120 &&
       vType->hasFloatingRepresentation())
     return InvalidOperands(Loc, LHS, RHS);
   
   return GetSignedVectorType(LHS.get()->getType());
 }
 
-inline QualType Sema::CheckBitwiseOperands(
-  ExprResult &LHS, ExprResult &RHS, SourceLocation Loc, bool IsCompAssign) {
+inline QualType Sema::CheckBitwiseOperands(ExprResult &LHS, ExprResult &RHS,
+                                           SourceLocation Loc,
+                                           BinaryOperatorKind Opc) {
   checkArithmeticNull(*this, LHS, RHS, Loc, /*isCompare=*/false);
 
+  bool IsCompAssign =
+      Opc == BO_AndAssign || Opc == BO_OrAssign || Opc == BO_XorAssign;
+
   if (LHS.get()->getType()->isVectorType() ||
       RHS.get()->getType()->isVectorType()) {
     if (LHS.get()->getType()->hasIntegerRepresentation() &&
         RHS.get()->getType()->hasIntegerRepresentation())
       return CheckVectorOperands(LHS, RHS, Loc, IsCompAssign,
                         /*AllowBothBool*/true,
                         /*AllowBoolConversions*/getLangOpts().ZVector);
     return InvalidOperands(Loc, LHS, RHS);
   }
 
+  if (Opc == BO_And)
+    diagnoseLogicalNotOnLHSofCheck(*this, LHS, RHS, Loc, Opc);
+
   ExprResult LHSResult = LHS, RHSResult = RHS;
   QualType compType = UsualArithmeticConversions(LHSResult, RHSResult,
                                                  IsCompAssign);
   if (LHSResult.isInvalid() || RHSResult.isInvalid())
     return QualType();
   LHS = LHSResult.get();
   RHS = RHSResult.get();
 
ExprResult Sema::CreateBuiltinBinOp(SourceLocation OpLoc,
   case BO_EQ:
   case BO_NE:
     ResultTy = CheckCompareOperands(LHS, RHS, OpLoc, Opc, false);
     break;
   case BO_And:
     checkObjCPointerIntrospection(*this, LHS, RHS, OpLoc);
   case BO_Xor:
   case BO_Or:
-    ResultTy = CheckBitwiseOperands(LHS, RHS, OpLoc);
+    ResultTy = CheckBitwiseOperands(LHS, RHS, OpLoc, Opc);
     break;
   case BO_LAnd:
   case BO_LOr:
     ResultTy = CheckLogicalOperands(LHS, RHS, OpLoc, Opc);
     break;
   case BO_MulAssign:
   case BO_DivAssign:
     CompResultTy = CheckMultiplyDivideOperands(LHS, RHS, OpLoc, true,
case BO_ShrAssign:
     CompLHSTy = CompResultTy;
     if (!CompResultTy.isNull() && !LHS.isInvalid() && !RHS.isInvalid())
       ResultTy = CheckAssignmentOperands(LHS.get(), RHS, OpLoc, CompResultTy);
     break;
   case BO_AndAssign:
   case BO_OrAssign: // fallthrough
     DiagnoseSelfAssignment(*this, LHS.get(), RHS.get(), OpLoc);
   case BO_XorAssign:
-    CompResultTy = CheckBitwiseOperands(LHS, RHS, OpLoc, true);
+    CompResultTy = CheckBitwiseOperands(LHS, RHS, OpLoc, Opc);
     CompLHSTy = CompResultTy;
     if (!CompResultTy.isNull() && !LHS.isInvalid() && !RHS.isInvalid())
       ResultTy = CheckAssignmentOperands(LHS.get(), RHS, OpLoc, CompResultTy);
     break;
   case BO_Comma:
     ResultTy = CheckCommaOperands(*this, LHS, RHS, OpLoc);
     if (getLangOpts().CPlusPlus && !RHS.isInvalid()) {
       VK = RHS.get()->getValueKind();

test/SemaCXX/warn-logical-not-compare.cpp

 // RUN: %clang_cc1 -fsyntax-only -Wlogical-not-parentheses -verify %s
-// RUN: %clang_cc1 -fsyntax-only -Wlogical-not-parentheses -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: not %clang_cc1 -fsyntax-only -Wlogical-not-parentheses -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
 
 bool getBool();
 int getInt();
 
 bool test1(int i1, int i2, bool b1, bool b2) {
   bool ret;
 
   ret = !i1 == i2;
bool test2 (E e) {
   ret = (!e) == e1;
   ret = (!e) == getE();
   ret = (!getE()) == e1;
   ret = (!getE()) == getE();
 
   return ret;
 }
 
+bool test_bitwise_op(int x) {
+  bool ret;
+
+  ret = !x & 1;
+  // expected-warning@-1 {{logical not is only applied to the left hand side of this bitwise operator}}
+  // expected-note@-2 {{add parentheses after the '!' to evaluate the bitwise operator first}}
+  // expected-note@-3 {{add parentheses around left hand side expression to silence this warning}}
+  // CHECK: warn-logical-not-compare.cpp:[[line:[0-9]*]]:9: warning
+  // CHECK: to evaluate the bitwise operator first
+  // CHECK: fix-it:"{{.*}}":{[[line]]:10-[[line]]:10}:"("
+  // CHECK: fix-it:"{{.*}}":{[[line]]:15-[[line]]:15}:")"
+  // CHECK: to silence this warning
+  // CHECK: fix-it:"{{.*}}":{[[line]]:9-[[line]]:9}:"("
+  // CHECK: fix-it:"{{.*}}":{[[line]]:11-[[line]]:11}:")"
+  ret = !(x & 1);
+  ret = (!x) & 1;
+
+  // This warning is really about !x & FOO since that's a common misspelling
+  // of the negated bit test !(x & FOO).  Don't warn for | and ^, since
+  // it's at least conceivable that the user wants to use | as an
+  // alternative to || that evaluates both branches.  (The warning above is
+  // only emitted if the operand to ! is not a bool, but in C that's common.)
+  // And there's no logical ^.
+  ret = !x | 1;
+  ret = !(x | 1);
+  ret = (!x) | 1;
+
+  ret = !x ^ 1;
+  ret = !(x ^ 1);
+  ret = (!x) ^ 1;
+
+  // These already err, don't also warn.
+  !x &= 1; // expected-error{{expression is not assignable}}
+  !x |= 1; // expected-error{{expression is not assignable}}
+  !x ^= 1; // expected-error{{expression is not assignable}}
+
+  return ret;
+}
+
 bool PR16673(int x) {
   bool ret;
   // Make sure we don't emit a fixit for the left paren, but not the right paren.
 #define X(x) x 
   ret = X(!x == 1 && 1);
   // expected-warning@-1 {{logical not is only applied to the left hand side of this comparison}}
   // expected-note@-2 {{add parentheses after the '!' to evaluate the comparison first}}
   // expected-note@-3 {{add parentheses around left hand side expression to silence this warning}}
   // CHECK: warn-logical-not-compare.cpp:[[line:[0-9]*]]:11: warning
   // CHECK: to evaluate the comparison first
   // CHECK-NOT: fix-it
   // CHECK: to silence this warning
   // CHECK-NOT: fix-it
   return ret;
 }