[clang-tidy][NFC] Enable 'readability-simplify-boolean-expr' check #158706
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: None (capitan-davide) ChangesCloses #156160 This is how the default configuration of this check looks like. Full diff: https://github.com/llvm/llvm-project/pull/158706.diff 13 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/.clang-tidy b/clang-tools-extra/clang-tidy/.clang-tidy
index d290901730405..4423f1c3dcd64 100644
--- a/clang-tools-extra/clang-tidy/.clang-tidy
+++ b/clang-tools-extra/clang-tidy/.clang-tidy
@@ -29,7 +29,6 @@ Checks: >
-readability-magic-numbers,
-readability-named-parameter,
-readability-qualified-auto,
- -readability-simplify-boolean-expr,
-readability-static-definition-in-anonymous-namespace,
-readability-suspicious-call-argument,
-readability-use-anyofallof
diff --git a/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
index c426b32ccade3..566de24ea3d1f 100644
--- a/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
@@ -1880,12 +1880,9 @@ static bool prefixSuffixCoverUnderThreshold(std::size_t Threshold,
padStringAtBegin(S1PadB, BiggerLength);
padStringAtBegin(S2PadB, BiggerLength);
- if (isCommonSuffixWithoutSomeCharacters(
- Threshold, StringRef{S1PadB.begin(), BiggerLength},
- StringRef{S2PadB.begin(), BiggerLength}))
- return true;
-
- return false;
+ return isCommonSuffixWithoutSomeCharacters(
+ Threshold, StringRef{S1PadB.begin(), BiggerLength},
+ StringRef{S2PadB.begin(), BiggerLength});
}
} // namespace filter
diff --git a/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
index 203170d55f694..1c54c96e83419 100644
--- a/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
@@ -268,10 +268,7 @@ static bool isDestCapacityOverflows(const MatchFinder::MatchResult &Result) {
// Assume that the destination array's capacity cannot overflow if the
// expression of the memory allocation contains '+ 1'.
StringRef DestCapacityExprStr = exprToStr(DestCapacityExpr, Result);
- if (DestCapacityExprStr.contains("+1") || DestCapacityExprStr.contains("+ 1"))
- return false;
-
- return true;
+ return !(DestCapacityExprStr.contains("+1") || DestCapacityExprStr.contains("+ 1"));
}
static bool
@@ -533,10 +530,7 @@ AST_MATCHER_P(Expr, hasDefinition, ast_matchers::internal::Matcher<Expr>,
hasLHS(declRefExpr(to(varDecl(equalsBoundNode(VarDeclName))))),
hasRHS(ignoringImpCasts(InnerMatcher))))))));
- if (DREHasDefinition.matches(*SimpleNode, Finder, Builder))
- return true;
-
- return false;
+ return DREHasDefinition.matches(*SimpleNode, Finder, Builder);
}
} // namespace
diff --git a/clang-tools-extra/clang-tidy/bugprone/SuspiciousEnumUsageCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SuspiciousEnumUsageCheck.cpp
index f2067bec001cc..fd5dca835b676 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SuspiciousEnumUsageCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SuspiciousEnumUsageCheck.cpp
@@ -100,7 +100,7 @@ static bool isPossiblyBitMask(const EnumDecl *EnumDec) {
return NonPowOfTwoCounter >= 1 && NonPowOfTwoCounter <= 2 &&
NonPowOfTwoCounter < EnumLen / 2 &&
(VR.MaxVal - VR.MinVal != EnumLen - 1) &&
- !(NonPowOfTwoCounter == 1 && isMaxValAllBitSetLiteral(EnumDec));
+ (NonPowOfTwoCounter != 1 || !isMaxValAllBitSetLiteral(EnumDec));
}
SuspiciousEnumUsageCheck::SuspiciousEnumUsageCheck(StringRef Name,
diff --git a/clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp b/clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp
index adcbf245ef7a3..cc1b01fa78908 100644
--- a/clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp
+++ b/clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp
@@ -27,10 +27,7 @@ static bool isMessageExpressionInsideMacro(const ObjCMessageExpr *Expr) {
return true;
SourceLocation SelectorLocation = Expr->getSelectorStartLoc();
- if (SelectorLocation.isMacroID())
- return true;
-
- return false;
+ return SelectorLocation.isMacroID();
}
// Walk up the class hierarchy looking for an -init method, returning true
diff --git a/clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp b/clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp
index c2fbc4422e5d2..6325f434be4ce 100644
--- a/clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp
+++ b/clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp
@@ -39,7 +39,7 @@ void ImplementationInNamespaceCheck::check(
}
// Enforce that the namespace is the result of macro expansion
- if (Result.SourceManager->isMacroBodyExpansion(NS->getLocation()) == false) {
+ if (!Result.SourceManager->isMacroBodyExpansion(NS->getLocation())) {
diag(NS->getLocation(), "the outermost namespace should be the '%0' macro")
<< RequiredNamespaceDeclMacroName;
return;
@@ -55,7 +55,7 @@ void ImplementationInNamespaceCheck::check(
}
// Lastly, make sure the namespace name actually has the __llvm_libc prefix
- if (NS->getName().starts_with(RequiredNamespaceRefStart) == false) {
+ if (!NS->getName().starts_with(RequiredNamespaceRefStart)) {
diag(NS->getLocation(), "the '%0' macro expansion should start with '%1'")
<< RequiredNamespaceDeclMacroName << RequiredNamespaceRefStart;
return;
diff --git a/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp b/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
index b32507d66cbac..18b3b8bdf9474 100644
--- a/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
@@ -54,8 +54,8 @@ ConstCorrectnessCheck::ConstCorrectnessCheck(StringRef Name,
AllowedTypes(
utils::options::parseStringList(Options.get("AllowedTypes", ""))) {
- if (AnalyzeValues == false && AnalyzeReferences == false &&
- AnalyzePointers == false)
+ if (!AnalyzeValues && !AnalyzeReferences &&
+ !AnalyzePointers)
this->configurationDiag(
"The check 'misc-const-correctness' will not "
"perform any analysis because 'AnalyzeValues', "
diff --git a/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp b/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
index 813005b892ed7..74ef936ff8e95 100644
--- a/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
@@ -70,7 +70,7 @@ IncludeCleanerCheck::IncludeCleanerCheck(StringRef Name,
IgnoreHeadersRegex.emplace_back(HeaderSuffix);
}
- if (UnusedIncludes == false && MissingIncludes == false)
+ if (!UnusedIncludes && !MissingIncludes)
this->configurationDiag("The check 'misc-include-cleaner' will not "
"perform any analysis because 'UnusedIncludes' and "
"'MissingIncludes' are both false.");
diff --git a/clang-tools-extra/clang-tidy/misc/NewDeleteOverloadsCheck.cpp b/clang-tools-extra/clang-tidy/misc/NewDeleteOverloadsCheck.cpp
index 2837f40bc49b8..89146e4cac0e8 100644
--- a/clang-tools-extra/clang-tidy/misc/NewDeleteOverloadsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/NewDeleteOverloadsCheck.cpp
@@ -52,11 +52,8 @@ AST_MATCHER(FunctionDecl, isPlacementOverload) {
const auto *FPT = Node.getType()->castAs<FunctionProtoType>();
ASTContext &Ctx = Node.getASTContext();
- if (Ctx.getLangOpts().SizedDeallocation &&
- Ctx.hasSameType(FPT->getParamType(1), Ctx.getSizeType()))
- return false;
-
- return true;
+ return !(Ctx.getLangOpts().SizedDeallocation &&
+ Ctx.hasSameType(FPT->getParamType(1), Ctx.getSizeType()));
}
} // namespace
diff --git a/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp b/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
index 107eda2e98f27..0a996aeffee02 100644
--- a/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
@@ -196,12 +196,9 @@ static bool areExclusiveRanges(BinaryOperatorKind OpcodeLHS,
// Handle the case where constants are off by one: x > 5 && x < 6.
APSInt ValueLhsPlus1;
- if (OpcodeLHS == BO_GT && OpcodeRHS == BO_LT &&
+ return OpcodeLHS == BO_GT && OpcodeRHS == BO_LT &&
incrementWithoutOverflow(ValueLHS, ValueLhsPlus1) &&
- APSInt::compareValues(ValueLhsPlus1, ValueRHS) == 0)
- return true;
-
- return false;
+ APSInt::compareValues(ValueLhsPlus1, ValueRHS) == 0;
}
// Returns whether the ranges covered by the union of both relational
@@ -726,12 +723,10 @@ static bool areSidesBinaryConstExpressions(const BinaryOperator *&BinOp,
return !E->isValueDependent() && E->isIntegerConstantExpr(*AstCtx);
};
- if ((IsIntegerConstantExpr(LhsBinOp->getLHS()) ||
+ return (IsIntegerConstantExpr(LhsBinOp->getLHS()) ||
IsIntegerConstantExpr(LhsBinOp->getRHS())) &&
(IsIntegerConstantExpr(RhsBinOp->getLHS()) ||
- IsIntegerConstantExpr(RhsBinOp->getRHS())))
- return true;
- return false;
+ IsIntegerConstantExpr(RhsBinOp->getRHS()));
}
static bool areSidesBinaryConstExpressionsOrDefinesOrIntegerConstant(
@@ -747,10 +742,8 @@ static bool areSidesBinaryConstExpressionsOrDefinesOrIntegerConstant(
auto IsDefineExpr = [AstCtx](const Expr *E) {
const SourceRange Lsr = E->getSourceRange();
- if (!Lsr.getBegin().isMacroID() || E->isValueDependent() ||
- !E->isIntegerConstantExpr(*AstCtx))
- return false;
- return true;
+ return !(!Lsr.getBegin().isMacroID() || E->isValueDependent() ||
+ !E->isIntegerConstantExpr(*AstCtx));
};
return IsDefineExpr(Lhs) || IsDefineExpr(Rhs);
diff --git a/clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
index 82f64096cbec1..2bb15169b4659 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
@@ -459,7 +459,7 @@ UseTrailingReturnTypeCheck::UseTrailingReturnTypeCheck(
TransformFunctions(Options.get("TransformFunctions", true)),
TransformLambdas(Options.get("TransformLambdas", TransformLambda::All)) {
- if (TransformFunctions == false && TransformLambdas == TransformLambda::None)
+ if (!TransformFunctions && TransformLambdas == TransformLambda::None)
this->configurationDiag(
"The check 'modernize-use-trailing-return-type' will not perform any "
"analysis because 'TransformFunctions' and 'TransformLambdas' are "
diff --git a/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp b/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
index 6b10e6b206a31..bd91fa6eec6c9 100644
--- a/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
@@ -209,7 +209,7 @@ getEquivalentForBoolLiteral(const CXXBoolLiteralExpr *BoolLiteral,
// Prior to C++11, false literal could be implicitly converted to pointer.
if (!Context.getLangOpts().CPlusPlus11 &&
(DestType->isPointerType() || DestType->isMemberPointerType()) &&
- BoolLiteral->getValue() == false) {
+ !BoolLiteral->getValue()) {
return "0";
}
diff --git a/clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp b/clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp
index a80637dee18f4..e9aeeba867ca7 100644
--- a/clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp
@@ -413,14 +413,11 @@ static bool areTypesCompatible(QualType ArgType, QualType ParamType,
// Arithmetic types are interconvertible, except scoped enums.
if (ParamType->isArithmeticType() && ArgType->isArithmeticType()) {
- if ((ParamType->isEnumeralType() && ParamType->castAsCanonical<EnumType>()
+ return !((ParamType->isEnumeralType() && ParamType->castAsCanonical<EnumType>()
->getOriginalDecl()
->isScoped()) ||
(ArgType->isEnumeralType() &&
- ArgType->castAsCanonical<EnumType>()->getOriginalDecl()->isScoped()))
- return false;
-
- return true;
+ ArgType->castAsCanonical<EnumType>()->getOriginalDecl()->isScoped()));
}
// Check if the argument and the param are both function types (the parameter
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
We generally include "NFC" in the title if it is a non-functional change to users. And please fix formatting issues. |
capitan-davide
changed the title
capitan-davide
force-pushed
the
enable-readability-simplify-boolean-expr
branch
2 times, most recently
from
c0abf32 to
821011d
Compare
Updated, also amended the commit message. |
capitan-davide
force-pushed
the
enable-readability-simplify-boolean-expr
branch
from
821011d to
4cc94d7
Compare
capitan-davide
force-pushed
the
enable-readability-simplify-boolean-expr
branch
from
4cc94d7 to
518ae97
Compare
vbvictor
requested review from
5chmidti,
EugeneZelenko,
HerrCai0907,
PiotrZSL,
carlosgalvezp,
localspook and
vbvictor
| !E->isIntegerConstantExpr(*AstCtx)) | ||
| return false; | ||
| return true; | ||
| return !(!Lsr.getBegin().isMacroID() || E->isValueDependent() || |
There was a problem hiding this comment.
For example, this transformation made this code a lot harder to understand
There was a problem hiding this comment.
Agree, IMO this more readable if we apply De Morgan here. We can even apply it manually and the check won't complain any longer (so, we can keep the default configuration in this case).
Original:
if (!Lsr.getBegin().isMacroID() || E->isValueDependent() ||
!E->isIntegerConstantExpr(*AstCtx))
return false;
return true;Before:
return !(!Lsr.getBegin().isMacroID() || E->isValueDependent() ||
!E->isIntegerConstantExpr(*AstCtx));After:
return Lsr.getBegin().isMacroID() && !E->isValueDependent() &&
E->isIntegerConstantExpr(*AstCtx));What do you think?
There was a problem hiding this comment.
Sorry for late reply, IMO DE-morgan looks better here. We could try it (I see there are places that it could be applied)
I agree, I have somewhat mixed feelings about this rule. Also negating conditions to make them more optimal/short can also lead to worse readability. I believe this is alleviated by setting |
I will do some experimentation the config options |
If we get unsatisfying results with it, we should think about making this check less aggressive (via new options) and leave this check disabled for now (But we could merge good transformations from it) |
I enabled |
Even though there are a number of |
In my particular case custom macro contained |
You have to build llvm with assertions enabled/in debug mode for that to happen (most LLVM docs will use Release by default since debug is a large build). |
What I mean is that currently, no assertion expression is getting refactored as it's probably not too complex |
6 participants
Closes #156160
This is how the default configuration of this check looks like.