[clang-tidy] Preserve comments and preprocessor directives when simplifying boolean expressions
This changeset still emits the diagnostic that the expression could be simplified, but it doesn't generate any fix-its that would lose comments or preprocessor directives within the text that would be replaced.
Fixes PR25842
Reviewers: alexfh
Subscribers: xazax.hun, cfe-commits
Patch by Richard Thomson! (+a naming style fix)
Differential Revision: http://reviews.llvm.org/D15737
llvm-svn: 256492
diff --git a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
index d8f467b..714874b 100644
--- a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
@@ -107,8 +107,12 @@
}
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) {
@@ -201,8 +205,7 @@
}
if (!NegatedOperator.empty() && LHS && RHS) {
return (asBool((getText(Result, *LHS) + " " + NegatedOperator + " " +
- getText(Result, *RHS))
- .str(),
+ getText(Result, *RHS)).str(),
NeedsStaticCast));
}
@@ -324,11 +327,10 @@
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,
@@ -347,15 +349,13 @@
if (ChainedConditionalReturn) {
Finder->addMatcher(ifStmt(isExpansionInMainFile(),
hasThen(returnsBool(Value, ThenLiteralId)),
- hasElse(returnsBool(!Value)))
- .bind(Id),
+ hasElse(returnsBool(!Value))).bind(Id),
this);
} else {
Finder->addMatcher(ifStmt(isExpansionInMainFile(),
unless(hasParent(ifStmt())),
hasThen(returnsBool(Value, ThenLiteralId)),
- hasElse(returnsBool(!Value)))
- .bind(Id),
+ hasElse(returnsBool(!Value))).bind(Id),
this);
}
}
@@ -382,8 +382,7 @@
} else {
Finder->addMatcher(ifStmt(isExpansionInMainFile(),
unless(hasParent(ifStmt())), hasThen(Then),
- hasElse(Else))
- .bind(Id),
+ hasElse(Else)).bind(Id),
this);
}
}
@@ -396,8 +395,7 @@
unless(hasElse(stmt())))),
hasAnySubstatement(
returnStmt(has(cxxBoolLiteral(equals(!Value))))
- .bind(CompoundReturnId))))
- .bind(Id),
+ .bind(CompoundReturnId)))).bind(Id),
this);
}
@@ -490,6 +488,39 @@
}
}
+bool containsDiscardedTokens(
+ const ast_matchers::MatchFinder::MatchResult &Result,
+ CharSourceRange CharRange) {
+ StringRef 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(
+ const ast_matchers::MatchFinder::MatchResult &Result, SourceLocation Loc,
+ StringRef Description, SourceRange ReplacementRange,
+ StringRef Replacement) {
+ CharSourceRange CharRange = Lexer::makeFileCharRange(
+ CharSourceRange::getTokenRange(ReplacementRange), *Result.SourceManager,
+ Result.Context->getLangOpts());
+
+ DiagnosticBuilder Diag = diag(Loc, Description);
+ if (!containsDiscardedTokens(Result, CharRange))
+ Diag << FixItHint::CreateReplacement(CharRange, Replacement);
+}
+
void SimplifyBooleanExprCheck::replaceWithExpression(
const ast_matchers::MatchFinder::MatchResult &Result,
const CXXBoolLiteralExpr *BoolLiteral, bool UseLHS, bool Negated) {
@@ -497,19 +528,18 @@
const auto *RHS = Result.Nodes.getNodeAs<Expr>(RHSId);
std::string Replacement =
replacementExpression(Result, Negated, UseLHS ? LHS : RHS);
- SourceLocation Start = LHS->getLocStart();
- SourceLocation End = RHS->getLocEnd();
- diag(BoolLiteral->getLocStart(), SimplifyOperatorDiagnostic)
- << FixItHint::CreateReplacement(SourceRange(Start, End), Replacement);
+ SourceRange Range(LHS->getLocStart(), RHS->getLocEnd());
+ issueDiag(Result, BoolLiteral->getLocStart(), SimplifyOperatorDiagnostic,
+ Range, Replacement);
}
void SimplifyBooleanExprCheck::replaceWithThenStatement(
const MatchFinder::MatchResult &Result,
const CXXBoolLiteralExpr *TrueConditionRemoved) {
const auto *IfStatement = Result.Nodes.getNodeAs<IfStmt>(IfStmtId);
- diag(TrueConditionRemoved->getLocStart(), SimplifyConditionDiagnostic)
- << FixItHint::CreateReplacement(IfStatement->getSourceRange(),
- getText(Result, *IfStatement->getThen()));
+ issueDiag(Result, TrueConditionRemoved->getLocStart(),
+ SimplifyConditionDiagnostic, IfStatement->getSourceRange(),
+ getText(Result, *IfStatement->getThen()));
}
void SimplifyBooleanExprCheck::replaceWithElseStatement(
@@ -517,10 +547,9 @@
const CXXBoolLiteralExpr *FalseConditionRemoved) {
const auto *IfStatement = Result.Nodes.getNodeAs<IfStmt>(IfStmtId);
const Stmt *ElseStatement = IfStatement->getElse();
- diag(FalseConditionRemoved->getLocStart(), SimplifyConditionDiagnostic)
- << FixItHint::CreateReplacement(
- IfStatement->getSourceRange(),
- ElseStatement ? getText(Result, *ElseStatement) : "");
+ issueDiag(Result, FalseConditionRemoved->getLocStart(),
+ SimplifyConditionDiagnostic, IfStatement->getSourceRange(),
+ ElseStatement ? getText(Result, *ElseStatement) : "");
}
void SimplifyBooleanExprCheck::replaceWithCondition(
@@ -528,9 +557,9 @@
bool Negated) {
std::string Replacement =
replacementExpression(Result, Negated, Ternary->getCond());
- diag(Ternary->getTrueExpr()->getLocStart(),
- "redundant boolean literal in ternary expression result")
- << FixItHint::CreateReplacement(Ternary->getSourceRange(), Replacement);
+ issueDiag(Result, Ternary->getTrueExpr()->getLocStart(),
+ "redundant boolean literal in ternary expression result",
+ Ternary->getSourceRange(), Replacement);
}
void SimplifyBooleanExprCheck::replaceWithReturnCondition(
@@ -540,8 +569,8 @@
std::string Replacement = ("return " + Condition + Terminator).str();
SourceLocation Start =
Result.Nodes.getNodeAs<CXXBoolLiteralExpr>(ThenLiteralId)->getLocStart();
- diag(Start, SimplifyConditionalReturnDiagnostic)
- << FixItHint::CreateReplacement(If->getSourceRange(), Replacement);
+ issueDiag(Result, Start, SimplifyConditionalReturnDiagnostic,
+ If->getSourceRange(), Replacement);
}
void SimplifyBooleanExprCheck::replaceCompoundReturnWithCondition(
@@ -570,10 +599,9 @@
const Expr *Condition = If->getCond();
std::string Replacement =
"return " + replacementExpression(Result, Negated, Condition);
- diag(Lit->getLocStart(), SimplifyConditionalReturnDiagnostic)
- << FixItHint::CreateReplacement(
- SourceRange(If->getLocStart(), Ret->getLocEnd()),
- Replacement);
+ issueDiag(
+ Result, Lit->getLocStart(), SimplifyConditionalReturnDiagnostic,
+ SourceRange(If->getLocStart(), Ret->getLocEnd()), Replacement);
return;
}
@@ -598,8 +626,9 @@
(VariableName + " = " + Condition + Terminator).str();
SourceLocation Location =
Result.Nodes.getNodeAs<CXXBoolLiteralExpr>(IfAssignLocId)->getLocStart();
- this->diag(Location, "redundant boolean literal in conditional assignment")
- << FixItHint::CreateReplacement(Range, Replacement);
+ issueDiag(Result, Location,
+ "redundant boolean literal in conditional assignment", Range,
+ Replacement);
}
} // namespace readability