njames93 created this revision.
njames93 added reviewers: aaron.ballman, LegalizeAdulthood.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a project: All.
njames93 requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D134592
Files:
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/readability/simplify-boolean-expr.rst
clang-tools-extra/test/clang-tidy/checkers/readability/simplifiy-bool-expr-floats.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/readability/simplifiy-bool-expr-floats.cpp
===================================================================
--- /dev/null
+++ 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;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability/simplify-boolean-expr.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/readability/simplify-boolean-expr.rst
+++ 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
+
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ 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 <clang-tidy/checks/readability/simplify-boolean-expr>`
+ check.
+
+ Added an option SafeFloatComparisons to control negating binary operators
+ with floating point operands.
+
Removed checks
^^^^^^^^^^^^^^
Index: clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
+++ 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
Index: clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
@@ -56,13 +56,21 @@
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 {};
}
@@ -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<ExprWithCleanups>(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<BinaryOperator>(E)) {
- NegatedOperator = negatedOperator(BinOp);
+ NegatedOperator = negatedOperator(BinOp, SafeFloatMode);
LHS = BinOp->getLHS();
RHS = BinOp->getRHS();
} else if (const auto *OpExpr = dyn_cast<CXXOperatorCallExpr>(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<BinaryOperator>(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<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,
@@ -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<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",
@@ -782,7 +796,8 @@
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
@@ -791,6 +806,7 @@
const ASTContext &Ctx,
const BinaryOperator *BinOp,
Optional<BinaryOperatorKind> 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<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())
@@ -878,11 +900,13 @@
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.
@@ -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;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
