diff --git a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h --- a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h +++ b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h @@ -70,6 +70,7 @@ const bool ChainedConditionalAssignment; const bool SimplifyDeMorgan; const bool SimplifyDeMorganRelaxed; + const bool SafeFloatComparisons; }; } // namespace readability diff --git a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp --- a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp @@ -56,13 +56,21 @@ static std::pair 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 {}; } @@ -160,7 +168,7 @@ } 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(E)) E = EC->getSubExpr(); @@ -175,7 +183,7 @@ if (needsZeroComparison(UnOp->getSubExpr())) return compareExpressionToZero(Context, UnOp->getSubExpr(), true); - return replacementExpression(Context, false, UnOp->getSubExpr()); + return replacementExpression(Context, false, UnOp->getSubExpr(), SafeFloatMode); } } @@ -189,7 +197,7 @@ const Expr *LHS = nullptr; const Expr *RHS = nullptr; if (const auto *BinOp = dyn_cast(E)) { - NegatedOperator = negatedOperator(BinOp); + NegatedOperator = negatedOperator(BinOp, SafeFloatMode); LHS = BinOp->getLHS(); RHS = BinOp->getRHS(); } else if (const auto *OpExpr = dyn_cast(E)) { @@ -516,7 +524,8 @@ 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(E->IgnoreUnlessSpelledInSource()); if (!BO) return false; @@ -529,14 +538,14 @@ 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; } @@ -561,7 +570,10 @@ 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()) { @@ -586,7 +598,8 @@ 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") @@ -633,7 +646,7 @@ 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); @@ -675,6 +688,7 @@ ChainedConditionalAssignment); Options.store(Opts, "SimplifyDeMorgan", SimplifyDeMorgan); Options.store(Opts, "SimplifyDeMorganRelaxed", SimplifyDeMorganRelaxed); + Options.store(Opts, "SafeFloatComparisons", SafeFloatComparisons); } void SimplifyBooleanExprCheck::registerMatchers(MatchFinder *Finder) { @@ -720,7 +734,7 @@ 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); @@ -731,7 +745,7 @@ bool Negated) { StringRef Terminator = isa(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, @@ -742,7 +756,7 @@ 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); @@ -757,7 +771,7 @@ StringRef VariableName = getText(Context, *Var); StringRef Terminator = isa(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", @@ -782,7 +796,8 @@ static bool flipDemorganSide(SmallVectorImpl &Fixes, const ASTContext &Ctx, const Expr *E, - Optional OuterBO); + Optional 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 @@ -791,6 +806,7 @@ const ASTContext &Ctx, const BinaryOperator *BinOp, Optional OuterBO, + bool SafeFloatMode, const ParenExpr *Parens = nullptr) { switch (BinOp->getOpcode()) { case BO_LAnd: @@ -825,8 +841,8 @@ ")")); } } - 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; }; @@ -836,14 +852,19 @@ case BO_GE: case BO_EQ: case BO_NE: - // 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; + // 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. @@ -868,7 +889,8 @@ static bool flipDemorganSide(SmallVectorImpl &Fixes, const ASTContext &Ctx, const Expr *E, - Optional OuterBO) { + Optional OuterBO, + bool SafeFloatMode) { if (isa(E) && cast(E)->getOpcode() == UO_LNot) { // if we have a not operator, '!a', just remove the '!'. if (cast(E)->getOperatorLoc().isMacroID()) @@ -878,11 +900,13 @@ return false; } if (const auto *BinOp = dyn_cast(E)) { - return flipDemorganBinaryOperator(Fixes, Ctx, BinOp, OuterBO); + return flipDemorganBinaryOperator(Fixes, Ctx, BinOp, OuterBO, + SafeFloatMode); } if (const auto *Paren = dyn_cast(E)) { if (const auto *BinOp = dyn_cast(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. @@ -948,8 +972,10 @@ } 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; diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -149,6 +149,12 @@ copy assignment operators with nonstandard return types. The check is restricted to c++11-or-later. +- Improved :doc:`readability-simplify-boolean-expr ` + check. + + Added an option SafeFloatComparisons to control negating binary operators + with floating point operands. + Removed checks ^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/simplify-boolean-expr.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/simplify-boolean-expr.rst --- a/clang-tools-extra/docs/clang-tidy/checks/readability/simplify-boolean-expr.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/readability/simplify-boolean-expr.rst @@ -117,3 +117,21 @@ 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 + diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/simplifiy-bool-expr-floats.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/simplifiy-bool-expr-floats.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/simplifiy-bool-expr-floats.cpp @@ -0,0 +1,19 @@ +// 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; +}