Reapply r260096.
Expand the simplify boolean expression check to handle implicit conversion of integral types to bool and improve the handling of implicit conversion of member pointers to bool.
Implicit conversion of member pointers are replaced with explicit comparisons to nullptr.
Implicit conversions of integral types are replaced with explicit comparisons to 0.
Patch by Richard Thomson.
llvm-svn: 260681
diff --git a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
index db09aaa..148eb14 100644
--- a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
@@ -85,10 +85,11 @@
E = E->IgnoreImpCasts();
if (isa<BinaryOperator>(E) || isa<ConditionalOperator>(E))
return true;
- if (const auto *Op = dyn_cast<CXXOperatorCallExpr>(E)) {
+
+ if (const auto *Op = dyn_cast<CXXOperatorCallExpr>(E))
return Op->getNumArgs() == 2 && Op->getOperator() != OO_Call &&
Op->getOperator() != OO_Subscript;
- }
+
return false;
}
@@ -107,12 +108,8 @@
}
std::pair<OverloadedOperatorKind, StringRef> OperatorNames[] = {
- {OO_EqualEqual, "=="},
- {OO_ExclaimEqual, "!="},
- {OO_Less, "<"},
- {OO_GreaterEqual, ">="},
- {OO_Greater, ">"},
- {OO_LessEqual, "<="}};
+ {OO_EqualEqual, "=="}, {OO_ExclaimEqual, "!="}, {OO_Less, "<"},
+ {OO_GreaterEqual, ">="}, {OO_Greater, ">"}, {OO_LessEqual, "<="}};
StringRef getOperatorName(OverloadedOperatorKind OpKind) {
for (auto Name : OperatorNames) {
@@ -148,7 +145,15 @@
bool needsNullPtrComparison(const Expr *E) {
if (const auto *ImpCast = dyn_cast<ImplicitCastExpr>(E))
- return ImpCast->getCastKind() == CK_PointerToBoolean;
+ return ImpCast->getCastKind() == CK_PointerToBoolean ||
+ ImpCast->getCastKind() == CK_MemberPointerToBoolean;
+
+ return false;
+}
+
+bool needsZeroComparison(const Expr *E) {
+ if (const auto *ImpCast = dyn_cast<ImplicitCastExpr>(E))
+ return ImpCast->getCastKind() == CK_IntegralToBoolean;
return false;
}
@@ -172,6 +177,29 @@
return !E->getType()->isBooleanType();
}
+std::string compareExpressionToConstant(const MatchFinder::MatchResult &Result,
+ const Expr *E, bool Negated,
+ const char *Constant) {
+ E = E->IgnoreImpCasts();
+ const std::string ExprText =
+ (isa<BinaryOperator>(E) ? ("(" + getText(Result, *E) + ")")
+ : getText(Result, *E))
+ .str();
+ return ExprText + " " + (Negated ? "!=" : "==") + " " + Constant;
+}
+
+std::string compareExpressionToNullPtr(const MatchFinder::MatchResult &Result,
+ const Expr *E, bool Negated) {
+ const char *NullPtr =
+ Result.Context->getLangOpts().CPlusPlus11 ? "nullptr" : "NULL";
+ return compareExpressionToConstant(Result, E, Negated, NullPtr);
+}
+
+std::string compareExpressionToZero(const MatchFinder::MatchResult &Result,
+ const Expr *E, bool Negated) {
+ return compareExpressionToConstant(Result, E, Negated, "0");
+}
+
std::string replacementExpression(const MatchFinder::MatchResult &Result,
bool Negated, const Expr *E) {
E = E->ignoreParenBaseCasts();
@@ -180,14 +208,20 @@
if (const auto *UnOp = dyn_cast<UnaryOperator>(E)) {
if (UnOp->getOpcode() == UO_LNot) {
if (needsNullPtrComparison(UnOp->getSubExpr()))
- return (getText(Result, *UnOp->getSubExpr()) + " != nullptr").str();
+ return compareExpressionToNullPtr(Result, UnOp->getSubExpr(), true);
+
+ if (needsZeroComparison(UnOp->getSubExpr()))
+ return compareExpressionToZero(Result, UnOp->getSubExpr(), true);
return replacementExpression(Result, false, UnOp->getSubExpr());
}
}
if (needsNullPtrComparison(E))
- return (getText(Result, *E) + " == nullptr").str();
+ return compareExpressionToNullPtr(Result, E, false);
+
+ if (needsZeroComparison(E))
+ return compareExpressionToZero(Result, E, false);
StringRef NegatedOperator;
const Expr *LHS = nullptr;
@@ -203,18 +237,21 @@
RHS = OpExpr->getArg(1);
}
}
- if (!NegatedOperator.empty() && LHS && RHS) {
+ if (!NegatedOperator.empty() && LHS && RHS)
return (asBool((getText(Result, *LHS) + " " + NegatedOperator + " " +
- getText(Result, *RHS)).str(),
+ getText(Result, *RHS))
+ .str(),
NeedsStaticCast));
- }
StringRef Text = getText(Result, *E);
if (!NeedsStaticCast && needsParensAfterUnaryNegation(E))
return ("!(" + Text + ")").str();
if (needsNullPtrComparison(E))
- return (getText(Result, *E) + " == nullptr").str();
+ return compareExpressionToNullPtr(Result, E, false);
+
+ if (needsZeroComparison(E))
+ return compareExpressionToZero(Result, E, false);
return ("!" + asBool(Text, NeedsStaticCast));
}
@@ -222,12 +259,18 @@
if (const auto *UnOp = dyn_cast<UnaryOperator>(E)) {
if (UnOp->getOpcode() == UO_LNot) {
if (needsNullPtrComparison(UnOp->getSubExpr()))
- return (getText(Result, *UnOp->getSubExpr()) + " == nullptr").str();
+ return compareExpressionToNullPtr(Result, UnOp->getSubExpr(), false);
+
+ if (needsZeroComparison(UnOp->getSubExpr()))
+ return compareExpressionToZero(Result, UnOp->getSubExpr(), false);
}
}
if (needsNullPtrComparison(E))
- return (getText(Result, *E) + " != nullptr").str();
+ return compareExpressionToNullPtr(Result, E, true);
+
+ if (needsZeroComparison(E))
+ return compareExpressionToZero(Result, E, true);
return asBool(getText(Result, *E), NeedsStaticCast);
}
@@ -258,6 +301,26 @@
return nullptr;
}
+bool containsDiscardedTokens(const MatchFinder::MatchResult &Result,
+ CharSourceRange CharRange) {
+ std::string ReplacementText =
+ Lexer::getSourceText(CharRange, *Result.SourceManager,
+ Result.Context->getLangOpts())
+ .str();
+ Lexer Lex(CharRange.getBegin(), Result.Context->getLangOpts(),
+ ReplacementText.data(), ReplacementText.data(),
+ ReplacementText.data() + ReplacementText.size());
+ Lex.SetCommentRetentionState(true);
+
+ Token Tok;
+ while (!Lex.LexFromRawLexer(Tok)) {
+ if (Tok.is(tok::TokenKind::comment) || Tok.is(tok::TokenKind::hash))
+ return true;
+ }
+
+ return false;
+}
+
} // namespace
SimplifyBooleanExprCheck::SimplifyBooleanExprCheck(StringRef Name,
@@ -301,12 +364,13 @@
StringRef OperatorName,
StringRef BooleanId) {
Finder->addMatcher(
- binaryOperator(isExpansionInMainFile(), hasOperatorName(OperatorName),
- hasLHS(allOf(expr().bind(LHSId),
- ignoringImpCasts(cxxBoolLiteral(equals(Value))
- .bind(BooleanId)))),
- hasRHS(expr().bind(RHSId)),
- unless(hasRHS(hasDescendant(cxxBoolLiteral())))),
+ binaryOperator(
+ isExpansionInMainFile(), hasOperatorName(OperatorName),
+ hasLHS(allOf(
+ expr().bind(LHSId),
+ ignoringImpCasts(cxxBoolLiteral(equals(Value)).bind(BooleanId)))),
+ hasRHS(expr().bind(RHSId)),
+ unless(hasRHS(hasDescendant(cxxBoolLiteral())))),
this);
}
@@ -315,22 +379,24 @@
StringRef OperatorName,
StringRef BooleanId) {
Finder->addMatcher(
- binaryOperator(isExpansionInMainFile(), hasOperatorName(OperatorName),
- unless(hasLHS(hasDescendant(cxxBoolLiteral()))),
- hasLHS(expr().bind(LHSId)),
- hasRHS(allOf(expr().bind(RHSId),
- ignoringImpCasts(cxxBoolLiteral(equals(Value))
- .bind(BooleanId))))),
+ binaryOperator(
+ isExpansionInMainFile(), hasOperatorName(OperatorName),
+ unless(hasLHS(hasDescendant(cxxBoolLiteral()))),
+ hasLHS(expr().bind(LHSId)),
+ hasRHS(allOf(expr().bind(RHSId),
+ ignoringImpCasts(
+ cxxBoolLiteral(equals(Value)).bind(BooleanId))))),
this);
}
void SimplifyBooleanExprCheck::matchBoolCondition(MatchFinder *Finder,
bool Value,
StringRef BooleanId) {
- Finder->addMatcher(ifStmt(isExpansionInMainFile(),
- hasCondition(cxxBoolLiteral(equals(Value))
- .bind(BooleanId))).bind(IfStmtId),
- this);
+ Finder->addMatcher(
+ ifStmt(isExpansionInMainFile(),
+ hasCondition(cxxBoolLiteral(equals(Value)).bind(BooleanId)))
+ .bind(IfStmtId),
+ this);
}
void SimplifyBooleanExprCheck::matchTernaryResult(MatchFinder *Finder,
@@ -346,18 +412,19 @@
void SimplifyBooleanExprCheck::matchIfReturnsBool(MatchFinder *Finder,
bool Value, StringRef Id) {
- if (ChainedConditionalReturn) {
+ if (ChainedConditionalReturn)
Finder->addMatcher(ifStmt(isExpansionInMainFile(),
hasThen(returnsBool(Value, ThenLiteralId)),
- hasElse(returnsBool(!Value))).bind(Id),
+ hasElse(returnsBool(!Value)))
+ .bind(Id),
this);
- } else {
+ else
Finder->addMatcher(ifStmt(isExpansionInMainFile(),
unless(hasParent(ifStmt())),
hasThen(returnsBool(Value, ThenLiteralId)),
- hasElse(returnsBool(!Value))).bind(Id),
+ hasElse(returnsBool(!Value)))
+ .bind(Id),
this);
- }
}
void SimplifyBooleanExprCheck::matchIfAssignsBool(MatchFinder *Finder,
@@ -375,16 +442,16 @@
hasRHS(cxxBoolLiteral(equals(!Value))));
auto Else = anyOf(SimpleElse, compoundStmt(statementCountIs(1),
hasAnySubstatement(SimpleElse)));
- if (ChainedConditionalAssignment) {
+ if (ChainedConditionalAssignment)
Finder->addMatcher(
ifStmt(isExpansionInMainFile(), hasThen(Then), hasElse(Else)).bind(Id),
this);
- } else {
+ else
Finder->addMatcher(ifStmt(isExpansionInMainFile(),
unless(hasParent(ifStmt())), hasThen(Then),
- hasElse(Else)).bind(Id),
+ hasElse(Else))
+ .bind(Id),
this);
- }
}
void SimplifyBooleanExprCheck::matchCompoundIfReturnsBool(MatchFinder *Finder,
@@ -395,7 +462,8 @@
unless(hasElse(stmt())))),
hasAnySubstatement(
returnStmt(has(cxxBoolLiteral(equals(!Value))))
- .bind(CompoundReturnId)))).bind(Id),
+ .bind(CompoundReturnId))))
+ .bind(Id),
this);
}
@@ -444,69 +512,46 @@
void SimplifyBooleanExprCheck::check(const MatchFinder::MatchResult &Result) {
if (const CXXBoolLiteralExpr *LeftRemoved =
- getBoolLiteral(Result, RightExpressionId)) {
+ getBoolLiteral(Result, RightExpressionId))
replaceWithExpression(Result, LeftRemoved, false);
- } else if (const CXXBoolLiteralExpr *RightRemoved =
- getBoolLiteral(Result, LeftExpressionId)) {
+ else if (const CXXBoolLiteralExpr *RightRemoved =
+ getBoolLiteral(Result, LeftExpressionId))
replaceWithExpression(Result, RightRemoved, true);
- } else if (const CXXBoolLiteralExpr *NegatedLeftRemoved =
- getBoolLiteral(Result, NegatedRightExpressionId)) {
+ else if (const CXXBoolLiteralExpr *NegatedLeftRemoved =
+ getBoolLiteral(Result, NegatedRightExpressionId))
replaceWithExpression(Result, NegatedLeftRemoved, false, true);
- } else if (const CXXBoolLiteralExpr *NegatedRightRemoved =
- getBoolLiteral(Result, NegatedLeftExpressionId)) {
+ else if (const CXXBoolLiteralExpr *NegatedRightRemoved =
+ getBoolLiteral(Result, NegatedLeftExpressionId))
replaceWithExpression(Result, NegatedRightRemoved, true, true);
- } else if (const CXXBoolLiteralExpr *TrueConditionRemoved =
- getBoolLiteral(Result, ConditionThenStmtId)) {
+ else if (const CXXBoolLiteralExpr *TrueConditionRemoved =
+ getBoolLiteral(Result, ConditionThenStmtId))
replaceWithThenStatement(Result, TrueConditionRemoved);
- } else if (const CXXBoolLiteralExpr *FalseConditionRemoved =
- getBoolLiteral(Result, ConditionElseStmtId)) {
+ else if (const CXXBoolLiteralExpr *FalseConditionRemoved =
+ getBoolLiteral(Result, ConditionElseStmtId))
replaceWithElseStatement(Result, FalseConditionRemoved);
- } else if (const auto *Ternary =
- Result.Nodes.getNodeAs<ConditionalOperator>(TernaryId)) {
+ else if (const auto *Ternary =
+ Result.Nodes.getNodeAs<ConditionalOperator>(TernaryId))
replaceWithCondition(Result, Ternary);
- } else if (const auto *TernaryNegated =
- Result.Nodes.getNodeAs<ConditionalOperator>(
- TernaryNegatedId)) {
+ else if (const auto *TernaryNegated =
+ Result.Nodes.getNodeAs<ConditionalOperator>(TernaryNegatedId))
replaceWithCondition(Result, TernaryNegated, true);
- } else if (const auto *If = Result.Nodes.getNodeAs<IfStmt>(IfReturnsBoolId)) {
+ else if (const auto *If = Result.Nodes.getNodeAs<IfStmt>(IfReturnsBoolId))
replaceWithReturnCondition(Result, If);
- } else if (const auto *IfNot =
- Result.Nodes.getNodeAs<IfStmt>(IfReturnsNotBoolId)) {
+ else if (const auto *IfNot =
+ Result.Nodes.getNodeAs<IfStmt>(IfReturnsNotBoolId))
replaceWithReturnCondition(Result, IfNot, true);
- } else if (const auto *IfAssign =
- Result.Nodes.getNodeAs<IfStmt>(IfAssignBoolId)) {
+ else if (const auto *IfAssign =
+ Result.Nodes.getNodeAs<IfStmt>(IfAssignBoolId))
replaceWithAssignment(Result, IfAssign);
- } else if (const auto *IfAssignNot =
- Result.Nodes.getNodeAs<IfStmt>(IfAssignNotBoolId)) {
+ else if (const auto *IfAssignNot =
+ Result.Nodes.getNodeAs<IfStmt>(IfAssignNotBoolId))
replaceWithAssignment(Result, IfAssignNot, true);
- } else if (const auto *Compound =
- Result.Nodes.getNodeAs<CompoundStmt>(CompoundBoolId)) {
+ else if (const auto *Compound =
+ Result.Nodes.getNodeAs<CompoundStmt>(CompoundBoolId))
replaceCompoundReturnWithCondition(Result, Compound);
- } else if (const auto *Compound =
- Result.Nodes.getNodeAs<CompoundStmt>(CompoundNotBoolId)) {
+ else if (const auto *Compound =
+ Result.Nodes.getNodeAs<CompoundStmt>(CompoundNotBoolId))
replaceCompoundReturnWithCondition(Result, Compound, true);
- }
-}
-
-bool containsDiscardedTokens(
- const ast_matchers::MatchFinder::MatchResult &Result,
- CharSourceRange CharRange) {
- std::string ReplacementText =
- Lexer::getSourceText(CharRange, *Result.SourceManager,
- Result.Context->getLangOpts())
- .str();
- Lexer Lex(CharRange.getBegin(), Result.Context->getLangOpts(),
- ReplacementText.data(), ReplacementText.data(),
- ReplacementText.data() + ReplacementText.size());
- Lex.SetCommentRetentionState(true);
- Token Tok;
-
- while (!Lex.LexFromRawLexer(Tok)) {
- if (Tok.is(tok::TokenKind::comment) || Tok.is(tok::TokenKind::hash))
- return true;
- }
-
- return false;
}
void SimplifyBooleanExprCheck::issueDiag(
@@ -582,7 +627,7 @@
// The body shouldn't be empty because the matcher ensures that it must
// contain at least two statements:
// 1) A `return` statement returning a boolean literal `false` or `true`
- // 2) An `if` statement with no `else` clause that consists fo a single
+ // 2) An `if` statement with no `else` clause that consists of a single
// `return` statement returning the opposite boolean literal `true` or
// `false`.
assert(Compound->size() >= 2);