LegalizeAdulthood created this revision.
LegalizeAdulthood added a reviewer: alexfh.
LegalizeAdulthood added a subscriber: cfe-commits.
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.
http://reviews.llvm.org/D15737
Files:
clang-tidy/readability/SimplifyBooleanExprCheck.cpp
clang-tidy/readability/SimplifyBooleanExprCheck.h
test/clang-tidy/readability-simplify-bool-expr.cpp
Index: test/clang-tidy/readability-simplify-bool-expr.cpp
===================================================================
--- test/clang-tidy/readability-simplify-bool-expr.cpp
+++ test/clang-tidy/readability-simplify-bool-expr.cpp
@@ -871,3 +871,23 @@
}
// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
// CHECK-FIXES: return p4 != nullptr;{{$}}
+
+bool comments_in_the_middle(bool b) {
+ if (b) {
+ return true;
+ } else {
+ // something wicked this way comes
+ return false;
+ }
+}
+// CHECK-MESSAGES: :[[@LINE-6]]:12: warning: {{.*}} in conditional return
+
+bool preprocessor_in_the_middle(bool b) {
+ if (b) {
+ return true;
+ } else {
+#define SOMETHING_WICKED false
+ return false;
+ }
+}
+// CHECK-MESSAGES: :[[@LINE-6]]:12: warning: {{.*}} in conditional return
Index: clang-tidy/readability/SimplifyBooleanExprCheck.h
===================================================================
--- clang-tidy/readability/SimplifyBooleanExprCheck.h
+++ clang-tidy/readability/SimplifyBooleanExprCheck.h
@@ -154,6 +154,10 @@
const ast_matchers::MatchFinder::MatchResult &Result,
const CompoundStmt *Compound, bool Negated = false);
+ void issueDiag(const ast_matchers::MatchFinder::MatchResult &Result,
+ SourceLocation Loc, StringRef Description,
+ SourceRange ReplacementRange, StringRef Replacement);
+
const bool ChainedConditionalReturn;
const bool ChainedConditionalAssignment;
};
Index: clang-tidy/readability/SimplifyBooleanExprCheck.cpp
===================================================================
--- clang-tidy/readability/SimplifyBooleanExprCheck.cpp
+++ 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,41 @@
}
}
+bool containsDiscardedTokens(
+ const ast_matchers::MatchFinder::MatchResult &Result, SourceRange Range) {
+ CharSourceRange CharRange = Lexer::makeFileCharRange(
+ CharSourceRange::getTokenRange(Range), *Result.SourceManager,
+ Result.Context->getLangOpts());
+
+ 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(
+ const ast_matchers::MatchFinder::MatchResult &Result, SourceLocation Loc,
+ StringRef Description, SourceRange ReplacementRange,
+ StringRef Replacement) {
+ if (containsDiscardedTokens(Result, ReplacementRange)) {
+ diag(Loc, Description);
+ } else
+ diag(Loc, Description) << FixItHint::CreateReplacement(ReplacementRange,
+ Replacement);
+}
+
void SimplifyBooleanExprCheck::replaceWithExpression(
const ast_matchers::MatchFinder::MatchResult &Result,
const CXXBoolLiteralExpr *BoolLiteral, bool UseLHS, bool Negated) {
@@ -499,38 +532,37 @@
replacementExpression(Result, Negated, UseLHS ? LHS : RHS);
SourceLocation Start = LHS->getLocStart();
SourceLocation End = RHS->getLocEnd();
- diag(BoolLiteral->getLocStart(), SimplifyOperatorDiagnostic)
- << FixItHint::CreateReplacement(SourceRange(Start, End), Replacement);
+ issueDiag(Result, BoolLiteral->getLocStart(), SimplifyOperatorDiagnostic,
+ SourceRange(Start, End), 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(
const MatchFinder::MatchResult &Result,
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(
const MatchFinder::MatchResult &Result, const ConditionalOperator *Ternary,
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 +572,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 +602,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 +629,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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
