[clang-tidy] Add option to control floating point binary operators in readability-simplify-boolean-expr

[njames93]

Summary

Negating a binary operator with floating point operands by negating the operator won't handle NaN comparisons correctly.
To address this an option has been added SafeFloatComparisons, defaults to being on, that will prevent this transformation.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

155

Please enclose SafeFloatComparisons into single back-ticks.

[njames93]

clang-tools-extra/docs/ReleaseNotes.rst

155

It would be nice if it were possible to link to the actual option documentation, but I dont think rst supports those kinds of links.

clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h

private:
   void issueDiag(const ASTContext &Result, SourceLocation Loc,
                  StringRef Description, SourceRange ReplacementRange,
                  StringRef Replacement);
 
   const bool ChainedConditionalReturn;
   const bool ChainedConditionalAssignment;
   const bool SimplifyDeMorgan;
   const bool SimplifyDeMorganRelaxed;
+  const bool SafeFloatComparisons;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_SIMPLIFY_BOOLEAN_EXPR_H

clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp

return Op->getNumArgs() == 2 && Op->getOperator() != OO_Call &&
            Op->getOperator() != OO_Subscript;
 
   return false;
 }
 
 static std::pair<BinaryOperatorKind, BinaryOperatorKind> Opposites[] = {
     {BO_LT, BO_GE}, {BO_GT, BO_LE}, {BO_EQ, BO_NE}};
 
-static StringRef negatedOperator(const BinaryOperator *BinOp) {
+bool hasFloatingOperand(const BinaryOperator *BO) {
+  return BO->getLHS()->getType()->isFloatingType() ||
+         BO->getLHS()->getType()->isFloatingType();
+}
+
+static StringRef negatedOperator(const BinaryOperator *BinOp,
+                                 bool SafeFloatMode) {
   const BinaryOperatorKind Opcode = BinOp->getOpcode();
+  if (SafeFloatMode && hasFloatingOperand(BinOp))
+    return {};
   for (auto NegatableOp : Opposites) {
     if (Opcode == NegatableOp.first)
-      return BinOp->getOpcodeStr(NegatableOp.second);
+      return BinaryOperator::getOpcodeStr(NegatableOp.second);
     if (Opcode == NegatableOp.second)
-      return BinOp->getOpcodeStr(NegatableOp.first);
+      return BinaryOperator::getOpcodeStr(NegatableOp.first);
   }
   return {};
 }
 
 static std::pair<OverloadedOperatorKind, StringRef> OperatorNames[] = {
     {OO_EqualEqual, "=="},   {OO_ExclaimEqual, "!="}, {OO_Less, "<"},
     {OO_GreaterEqual, ">="}, {OO_Greater, ">"},       {OO_LessEqual, "<="}};
 

 }
 
 static std::string compareExpressionToZero(const ASTContext &Context,
                                            const Expr *E, bool Negated) {
   return compareExpressionToConstant(Context, E, Negated, "0");
 }
 
 static std::string replacementExpression(const ASTContext &Context,
-                                         bool Negated, const Expr *E) {
+                                         bool Negated, const Expr *E, bool SafeFloatMode) {
   E = E->IgnoreParenBaseCasts();
   if (const auto *EC = dyn_cast<ExprWithCleanups>(E))
     E = EC->getSubExpr();
 
   const bool NeedsStaticCast = needsStaticCast(E);
   if (Negated) {
     if (const auto *UnOp = dyn_cast<UnaryOperator>(E)) {
       if (UnOp->getOpcode() == UO_LNot) {
         if (needsNullPtrComparison(UnOp->getSubExpr()))
           return compareExpressionToNullPtr(Context, UnOp->getSubExpr(), true);
 
         if (needsZeroComparison(UnOp->getSubExpr()))
           return compareExpressionToZero(Context, UnOp->getSubExpr(), true);
 
-        return replacementExpression(Context, false, UnOp->getSubExpr());
+        return replacementExpression(Context, false, UnOp->getSubExpr(), SafeFloatMode);
       }
     }
 
     if (needsNullPtrComparison(E))
       return compareExpressionToNullPtr(Context, E, false);
 
     if (needsZeroComparison(E))
       return compareExpressionToZero(Context, E, false);
 
     StringRef NegatedOperator;
     const Expr *LHS = nullptr;
     const Expr *RHS = nullptr;
     if (const auto *BinOp = dyn_cast<BinaryOperator>(E)) {
-      NegatedOperator = negatedOperator(BinOp);
+      NegatedOperator = negatedOperator(BinOp, SafeFloatMode);
       LHS = BinOp->getLHS();
       RHS = BinOp->getRHS();
     } else if (const auto *OpExpr = dyn_cast<CXXOperatorCallExpr>(E)) {
       if (OpExpr->getNumArgs() == 2) {
         NegatedOperator = negatedOperator(OpExpr);
         LHS = OpExpr->getArg(0);
         RHS = OpExpr->getArg(1);
       }
return isa<UnaryOperator>(E) &&
            cast<UnaryOperator>(E)->getOpcode() == UO_LNot;
   }
 
   template <typename Functor>
   static bool checkEitherSide(const BinaryOperator *BO, Functor Func) {
     return Func(BO->getLHS()) || Func(BO->getRHS());
   }
 
-  static bool nestedDemorgan(const Expr *E, unsigned NestingLevel) {
+  static bool nestedDemorgan(const Expr *E, unsigned NestingLevel,
+                             bool SafeFloatMode) {
     const auto *BO = dyn_cast<BinaryOperator>(E->IgnoreUnlessSpelledInSource());
     if (!BO)
       return false;
     if (!BO->getType()->isBooleanType())
       return false;
     switch (BO->getOpcode()) {
     case BO_LT:
     case BO_GT:
     case BO_LE:
     case BO_GE:
     case BO_EQ:
     case BO_NE:
-      return true;
+      return !SafeFloatMode || !hasFloatingOperand(BO);
     case BO_LAnd:
     case BO_LOr:
       if (checkEitherSide(BO, isUnaryLNot))
         return true;
       if (NestingLevel) {
-        if (checkEitherSide(BO, [NestingLevel](const Expr *E) {
-              return nestedDemorgan(E, NestingLevel - 1);
+        if (checkEitherSide(BO, [NestingLevel, SafeFloatMode](const Expr *E) {
+              return nestedDemorgan(E, NestingLevel - 1, SafeFloatMode);
             }))
           return true;
       }
       return false;
     default:
       return false;
     }
   }
 
   bool TraverseUnaryOperator(UnaryOperator *Op) {
     if (!Check->SimplifyDeMorgan || Op->getOpcode() != UO_LNot)
       return Base::TraverseUnaryOperator(Op);
     Expr *SubImp = Op->getSubExpr()->IgnoreImplicit();
     auto *Parens = dyn_cast<ParenExpr>(SubImp);
     auto *BinaryOp =
         Parens
             ? dyn_cast<BinaryOperator>(Parens->getSubExpr()->IgnoreImplicit())
             : dyn_cast<BinaryOperator>(SubImp);
     if (!BinaryOp || !BinaryOp->isLogicalOp() ||
         !BinaryOp->getType()->isBooleanType())
       return Base::TraverseUnaryOperator(Op);
     if (Check->SimplifyDeMorganRelaxed ||
         checkEitherSide(BinaryOp, isUnaryLNot) ||
         checkEitherSide(BinaryOp,
-                        [](const Expr *E) { return nestedDemorgan(E, 1); })) {
+                        [SafeFloatComparisons =
+                             Check->SafeFloatComparisons](const Expr *E) {
+                          return nestedDemorgan(E, 1, SafeFloatComparisons);
+                        })) {
       if (Check->reportDeMorgan(Context, Op, BinaryOp, !IsProcessing, parent(),
                                 Parens) &&
           !Check->areDiagsSelfContained()) {
         llvm::SaveAndRestore<bool> RAII(IsProcessing, true);
         return Base::TraverseUnaryOperator(Op);
       }
     }
     return Base::TraverseUnaryOperator(Op);
   }
 
 private:
   bool IsProcessing = false;
   SimplifyBooleanExprCheck *Check;
   SmallVector<Stmt *, 32> StmtStack;
   ASTContext &Context;
 };
 
 SimplifyBooleanExprCheck::SimplifyBooleanExprCheck(StringRef Name,
                                                    ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       ChainedConditionalReturn(Options.get("ChainedConditionalReturn", false)),
       ChainedConditionalAssignment(
           Options.get("ChainedConditionalAssignment", false)),
       SimplifyDeMorgan(Options.get("SimplifyDeMorgan", true)),
-      SimplifyDeMorganRelaxed(Options.get("SimplifyDeMorganRelaxed", false)) {
+      SimplifyDeMorganRelaxed(Options.get("SimplifyDeMorganRelaxed", false)),
+      SafeFloatComparisons(Options.get("SafeFloatComparisons", true)) {
   if (SimplifyDeMorganRelaxed && !SimplifyDeMorgan)
     configurationDiag("%0: 'SimplifyDeMorganRelaxed' cannot be enabled "
                       "without 'SimplifyDeMorgan' enabled")
         << Name;
 }
 
 static bool containsBoolLiteral(const Expr *E) {
   if (!E)
void SimplifyBooleanExprCheck::reportBinOp(const ASTContext &Context,
   if (!isa<CXXBoolLiteralExpr>(Other) && containsBoolLiteral(Other))
     return;
 
   bool BoolValue = Bool->getValue();
 
   auto ReplaceWithExpression = [this, &Context, LHS, RHS,
                                 Bool](const Expr *ReplaceWith, bool Negated) {
     std::string Replacement =
-        replacementExpression(Context, Negated, ReplaceWith);
+        replacementExpression(Context, Negated, ReplaceWith, SafeFloatComparisons);
     SourceRange Range(LHS->getBeginLoc(), RHS->getEndLoc());
     issueDiag(Context, Bool->getBeginLoc(), SimplifyOperatorDiagnostic, Range,
               Replacement);
   };
 
   switch (Op->getOpcode()) {
   case BO_LAnd:
     if (BoolValue)

 }
 
 void SimplifyBooleanExprCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "ChainedConditionalReturn", ChainedConditionalReturn);
   Options.store(Opts, "ChainedConditionalAssignment",
                 ChainedConditionalAssignment);
   Options.store(Opts, "SimplifyDeMorgan", SimplifyDeMorgan);
   Options.store(Opts, "SimplifyDeMorganRelaxed", SimplifyDeMorganRelaxed);
+  Options.store(Opts, "SafeFloatComparisons", SafeFloatComparisons);
 }
 
 void SimplifyBooleanExprCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(translationUnitDecl(), this);
 }
 
 void SimplifyBooleanExprCheck::check(const MatchFinder::MatchResult &Result) {
   Visitor(this, *Result.Context).traverse();
issueDiag(Context, BoolLiteral->getBeginLoc(), SimplifyConditionDiagnostic,
             IfStatement->getSourceRange(),
             ElseStatement ? getText(Context, *ElseStatement) : "");
 }
 
 void SimplifyBooleanExprCheck::replaceWithCondition(
     const ASTContext &Context, const ConditionalOperator *Ternary,
     bool Negated) {
   std::string Replacement =
-      replacementExpression(Context, Negated, Ternary->getCond());
+      replacementExpression(Context, Negated, Ternary->getCond(), SafeFloatComparisons);
   issueDiag(Context, Ternary->getTrueExpr()->getBeginLoc(),
             "redundant boolean literal in ternary expression result",
             Ternary->getSourceRange(), Replacement);
 }
 
 void SimplifyBooleanExprCheck::replaceWithReturnCondition(
     const ASTContext &Context, const IfStmt *If, const Expr *BoolLiteral,
     bool Negated) {
   StringRef Terminator = isa<CompoundStmt>(If->getElse()) ? ";" : "";
   std::string Condition =
-      replacementExpression(Context, Negated, If->getCond());
+      replacementExpression(Context, Negated, If->getCond(), SafeFloatComparisons);
   std::string Replacement = ("return " + Condition + Terminator).str();
   SourceLocation Start = BoolLiteral->getBeginLoc();
   issueDiag(Context, Start, SimplifyConditionalReturnDiagnostic,
             If->getSourceRange(), Replacement);
 }
 
 void SimplifyBooleanExprCheck::replaceCompoundReturnWithCondition(
     const ASTContext &Context, const ReturnStmt *Ret, bool Negated,
     const IfStmt *If, const Expr *ThenReturn) {
   const std::string Replacement =
-      "return " + replacementExpression(Context, Negated, If->getCond());
+      "return " + replacementExpression(Context, Negated, If->getCond(), SafeFloatComparisons);
   issueDiag(Context, ThenReturn->getBeginLoc(),
             SimplifyConditionalReturnDiagnostic,
             SourceRange(If->getBeginLoc(), Ret->getEndLoc()), Replacement);
 }
 
 void SimplifyBooleanExprCheck::replaceWithAssignment(const ASTContext &Context,
                                                      const IfStmt *IfAssign,
                                                      const Expr *Var,
                                                      SourceLocation Loc,
                                                      bool Negated) {
   SourceRange Range = IfAssign->getSourceRange();
   StringRef VariableName = getText(Context, *Var);
   StringRef Terminator = isa<CompoundStmt>(IfAssign->getElse()) ? ";" : "";
   std::string Condition =
-      replacementExpression(Context, Negated, IfAssign->getCond());
+      replacementExpression(Context, Negated, IfAssign->getCond(), SafeFloatComparisons);
   std::string Replacement =
       (VariableName + " = " + Condition + Terminator).str();
   issueDiag(Context, Loc, "redundant boolean literal in conditional assignment",
             Range, Replacement);
 }
 
 /// Swaps a \c BinaryOperator opcode from `&&` to `||` or vice-versa.
 static bool flipDemorganOperator(llvm::SmallVectorImpl<FixItHint> &Output,
                                  const BinaryOperator *BO) {
   assert(BO->isLogicalOp());
   if (BO->getOperatorLoc().isMacroID())
     return true;
   Output.push_back(FixItHint::CreateReplacement(
       BO->getOperatorLoc(), BO->getOpcode() == BO_LAnd ? "||" : "&&"));
   return false;
 }
 
 static BinaryOperatorKind getDemorganFlippedOperator(BinaryOperatorKind BO) {
   assert(BinaryOperator::isLogicalOp(BO));
   return BO == BO_LAnd ? BO_LOr : BO_LAnd;
 }
 
 static bool flipDemorganSide(SmallVectorImpl<FixItHint> &Fixes,
                              const ASTContext &Ctx, const Expr *E,
-                             Optional<BinaryOperatorKind> OuterBO);
+                             Optional<BinaryOperatorKind> OuterBO,
+                             bool SafeFloatMode);
 
 /// Inverts \p BinOp, Removing \p Parens if they exist and are safe to remove.
 /// returns \c true if there is any issue building the Fixes, \c false
 /// otherwise.
 static bool flipDemorganBinaryOperator(SmallVectorImpl<FixItHint> &Fixes,
                                        const ASTContext &Ctx,
                                        const BinaryOperator *BinOp,
                                        Optional<BinaryOperatorKind> OuterBO,
+                                       bool SafeFloatMode,
                                        const ParenExpr *Parens = nullptr) {
   switch (BinOp->getOpcode()) {
   case BO_LAnd:
   case BO_LOr: {
     // if we have 'a && b' or 'a || b', use demorgan to flip it to '!a || !b'
     // or '!a && !b'.
     if (flipDemorganOperator(Fixes, BinOp))
       return true;
if (OuterBO) {
         Fixes.push_back(FixItHint::CreateInsertion(BinOp->getBeginLoc(), "("));
         Fixes.push_back(FixItHint::CreateInsertion(
             Lexer::getLocForEndOfToken(BinOp->getEndLoc(), 0,
                                        Ctx.getSourceManager(),
                                        Ctx.getLangOpts()),
             ")"));
       }
     }
-    if (flipDemorganSide(Fixes, Ctx, BinOp->getLHS(), NewOp) ||
-        flipDemorganSide(Fixes, Ctx, BinOp->getRHS(), NewOp))
+    if (flipDemorganSide(Fixes, Ctx, BinOp->getLHS(), NewOp, SafeFloatMode) ||
+        flipDemorganSide(Fixes, Ctx, BinOp->getRHS(), NewOp, SafeFloatMode))
       return true;
     return false;
   };
   case BO_LT:
   case BO_GT:
   case BO_LE:
   case BO_GE:
   case BO_EQ:
   case BO_NE:
+    // Negating the binary operator doesn't handle floating point comparisons
+    // with NaN.
+    if (!SafeFloatMode || !hasFloatingOperand(BinOp)) {
       // For comparison operators, just negate the comparison.
       if (BinOp->getOperatorLoc().isMacroID())
         return true;
       Fixes.push_back(FixItHint::CreateReplacement(
           BinOp->getOperatorLoc(),
           BinaryOperator::getOpcodeStr(
               BinaryOperator::negateComparisonOp(BinOp->getOpcode()))));
       return false;
+    }
+    LLVM_FALLTHROUGH;
   default:
     // for any other binary operator, just use logical not and wrap in
     // parens.
     if (Parens) {
       if (Parens->getBeginLoc().isMacroID())
         return true;
       Fixes.push_back(FixItHint::CreateInsertion(Parens->getBeginLoc(), "!"));
     } else {
       if (BinOp->getBeginLoc().isMacroID() || BinOp->getEndLoc().isMacroID())
         return true;
       Fixes.append({FixItHint::CreateInsertion(BinOp->getBeginLoc(), "!("),
                     FixItHint::CreateInsertion(
                         Lexer::getLocForEndOfToken(BinOp->getEndLoc(), 0,
                                                    Ctx.getSourceManager(),
                                                    Ctx.getLangOpts()),
                         ")")});
     }
     break;
   }
   return false;
 }
 
 static bool flipDemorganSide(SmallVectorImpl<FixItHint> &Fixes,
                              const ASTContext &Ctx, const Expr *E,
-                             Optional<BinaryOperatorKind> OuterBO) {
+                             Optional<BinaryOperatorKind> OuterBO,
+                             bool SafeFloatMode) {
   if (isa<UnaryOperator>(E) && cast<UnaryOperator>(E)->getOpcode() == UO_LNot) {
     //  if we have a not operator, '!a', just remove the '!'.
     if (cast<UnaryOperator>(E)->getOperatorLoc().isMacroID())
       return true;
     Fixes.push_back(
         FixItHint::CreateRemoval(cast<UnaryOperator>(E)->getOperatorLoc()));
     return false;
   }
   if (const auto *BinOp = dyn_cast<BinaryOperator>(E)) {
-    return flipDemorganBinaryOperator(Fixes, Ctx, BinOp, OuterBO);
+    return flipDemorganBinaryOperator(Fixes, Ctx, BinOp, OuterBO,
+                                      SafeFloatMode);
   }
   if (const auto *Paren = dyn_cast<ParenExpr>(E)) {
     if (const auto *BinOp = dyn_cast<BinaryOperator>(Paren->getSubExpr())) {
-      return flipDemorganBinaryOperator(Fixes, Ctx, BinOp, OuterBO, Paren);
+      return flipDemorganBinaryOperator(Fixes, Ctx, BinOp, OuterBO,
+                                        SafeFloatMode, Paren);
     }
   }
   // Fallback case just insert a logical not operator.
   if (E->getBeginLoc().isMacroID())
     return true;
   Fixes.push_back(FixItHint::CreateInsertion(E->getBeginLoc(), "!"));
   return false;
 }
if (shouldRemoveParens(Parent, NewOpcode, Parens)) {
     Fixes.push_back(FixItHint::CreateRemoval(
         SourceRange(Outer->getOperatorLoc(), Parens->getLParen())));
     Fixes.push_back(FixItHint::CreateRemoval(Parens->getRParen()));
   } else {
     Fixes.push_back(FixItHint::CreateRemoval(Outer->getOperatorLoc()));
   }
   if (flipDemorganOperator(Fixes, Inner))
     return false;
-  if (flipDemorganSide(Fixes, Context, Inner->getLHS(), NewOpcode) ||
-      flipDemorganSide(Fixes, Context, Inner->getRHS(), NewOpcode))
+  if (flipDemorganSide(Fixes, Context, Inner->getLHS(), NewOpcode,
+                       SafeFloatComparisons) ||
+      flipDemorganSide(Fixes, Context, Inner->getRHS(), NewOpcode,
+                       SafeFloatComparisons))
     return false;
   Diag << Fixes;
   return true;
 }
 } // namespace readability
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 - Improved :doc:`modernize-use-equals-default <clang-tidy/checks/modernize/use-equals-default>`
   check.
 
   The check now skips unions/union-like classes since in this case a default constructor
   with empty body is not equivalent to the explicitly defaulted one. The check also skips
   copy assignment operators with nonstandard return types. The check is restricted to
   c++11-or-later.
 
+- Improved :doc:`readability-simplify-boolean-expr <clang-tidy/checks/readability/simplify-boolean-expr>`
+  check.
+
+  Added an option SafeFloatComparisons to control negating binary operators 
+  with floating point operands.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

clang-tools-extra/docs/clang-tidy/checks/readability/simplify-boolean-expr.rst

.. code-block::
       bool Y = !(A || B)
 
    Would be transformed to:
 
    .. code-block::
 
       bool X = !A || !B
       bool Y = !A && !B
+
+.. option:: SafeFloatComparisons
+
+   If `true`, The check wont negate binary operators with floating point 
+   operands by negating the operator.
+
+   This is because comparisons with NaN values will always return ``false``.
+
+   Default value is `true`.
+
+   .. code-block:: c++
+
+      if (FloatVar > 0.0f) return false; 
+      else return true;
+
+      return !(FloatVar > 0.0f); // If true
+      return FloatVar <= 0.0f;   // If false
+

clang-tools-extra/test/clang-tidy/checkers/readability/simplifiy-bool-expr-floats.cpp

+// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t -check-suffixes=',SAFE'
+// RUN: %check_clang_tidy %s readability-simplify-boolean-expr %t -check-suffixes=',UNSAFE' \
+// RUN:  -config="{CheckOptions: {readability-simplify-boolean-expr.SafeFloatComparisons: false}}" --
+
+// check_clang_tidy still passes CHECK-FIXES as a prefix, and we can't use `--allow-unused-prefixes`
+// CHECK-FIXES: bool a(float X, bool B){
+
+bool a(float X, bool B){
+
+  // CHECK-MESSAGES-UNSAFE: :[[@LINE+1]]:12: warning: boolean expression can be simplified by DeMorgan's theorem 
+  bool R = !(X < 0.0f && B);
+  // CHECK-FIXES-UNSAFE: bool R = X >= 0.0f || !B;
+
+  // CHECK-MESSAGES: :[[@LINE+1]]:24: warning: redundant boolean literal in conditional return statement
+  if (X > 0.0f) return false;
+    return true;
+  // CHECK-FIXES-SAFE: return !(X > 0.0f);
+  // CHECK-FIXES-UNSAFE: return X <= 0.0f;
+}