[clang-tidy] Preserve comments and preprocessor directives when simplifying boolean expressions

[LegalizeAdulthood]

Summary

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

[alexfh]

Thank you for the fix. See the comments inline.

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

493

The range should be converted to file char range before passing it to FixItHint::CreateReplacement, otherwise the fix may be applied incorrectly (or not applied at all). So please move this call to issueDiag and pass a CharSourceRange here.

497

Please use StringRef to avoid unnecessary string allocation and copying.

507

It's more common in this code to omit braces around single-line if bodies.

519

A few issues here:

1. range passed to FixItHint::CreateReplacement should be a file range, so the makeFileCharRange call should be moved here;
2. you can store the result of the diag(...) call to avoid code duplication;
3. inconsistently used braces.

CharSourceRange CharRange = Lexer::makeFileCharRange(
    CharSourceRange::getTokenRange(ReplacementRange),
    *Result.SourceManager, Result.Context->getLangOpts());
auto Diag = diag(Loc, Description);
if (!containsDiscardedTokens(Result, CharRange))
  Diag << FixItHint::CreateReplacement(CharRange, Replacement);

530–531

I'd slightly prefer to create a SourceRange right away:

SourceRange Range(LHS->getLocStart(), RHS->getLocEnd());
issueDiag(..., Range, ...);

test/clang-tidy/readability-simplify-bool-expr.cpp

883

This test doesn't verify that the code isn't changed. I suspect, it already passes without changing the check.

Please change b to a unique name (say, b10) and add

// CHECK-FIXES: {{^}}  if (b10) {
// CHECK-FIXES: {{^}}    // something wicked this way comes{{$}}

Same below.

[LegalizeAdulthood]

Update from review comments

[alexfh]

Thanks!

Looks good with one nit. Do you need me to commit the patch for you?

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

519

The name diagnostic violates LLVM naming rules. Should be Diagnostic (or Diag, since this variant is consistent with the corresponding function name and used widely across clang-tidy code).

[alexfh]

Do you need me to commit the patch for you?

Assuming "yes", since I don't recall any patches committed by you. Actually, it might be a good idea for you to request commit access at this point.

clang-tidy/readability/SimplifyBooleanExprCheck.h

private:
   void
   replaceWithAssignment(const ast_matchers::MatchFinder::MatchResult &Result,
                         const IfStmt *If, bool Negated = false);
 
   void replaceCompoundReturnWithCondition(
       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;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_SIMPLIFY_BOOLEAN_EXPR_H

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

if (Opcode == NegatableOp.first)
       return BinOp->getOpcodeStr(NegatableOp.second);
     if (Opcode == NegatableOp.second)
       return BinOp->getOpcodeStr(NegatableOp.first);
   }
   return StringRef();
 }
 
 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) {
     if (Name.first == OpKind)
       return Name.second;
   }
 
   return StringRef();
if (const auto *BinOp = dyn_cast<BinaryOperator>(E)) {
       if (OpExpr->getNumArgs() == 2) {
         NegatedOperator = negatedOperator(OpExpr);
         LHS = OpExpr->getArg(0);
         RHS = OpExpr->getArg(1);
       }
     }
     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))
Finder->addMatcher(
                                   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),
+  Finder->addMatcher(ifStmt(isExpansionInMainFile(),
+                            hasCondition(cxxBoolLiteral(equals(Value))
+                                             .bind(BooleanId))).bind(IfStmtId),
                      this);
 }
 
 void SimplifyBooleanExprCheck::matchTernaryResult(MatchFinder *Finder,
                                                   bool Value,
                                                   StringRef TernaryId) {
   Finder->addMatcher(
       conditionalOperator(isExpansionInMainFile(),
                           hasTrueExpression(cxxBoolLiteral(equals(Value))),
                           hasFalseExpression(cxxBoolLiteral(equals(!Value))))
           .bind(TernaryId),
       this);
 }
 
 void SimplifyBooleanExprCheck::matchIfReturnsBool(MatchFinder *Finder,
                                                   bool Value, StringRef Id) {
   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);
   }
 }
 
 void SimplifyBooleanExprCheck::matchIfAssignsBool(MatchFinder *Finder,
                                                   bool Value, StringRef Id) {
   auto SimpleThen = binaryOperator(
       hasOperatorName("="),
auto Else = anyOf(SimpleElse, compoundStmt(statementCountIs(1),
                                              hasAnySubstatement(SimpleElse)));
   if (ChainedConditionalAssignment) {
     Finder->addMatcher(
         ifStmt(isExpansionInMainFile(), hasThen(Then), hasElse(Else)).bind(Id),
         this);
   } else {
     Finder->addMatcher(ifStmt(isExpansionInMainFile(),
                               unless(hasParent(ifStmt())), hasThen(Then),
-                              hasElse(Else))
-                           .bind(Id),
+                              hasElse(Else)).bind(Id),
                        this);
   }
 }
 
 void SimplifyBooleanExprCheck::matchCompoundIfReturnsBool(MatchFinder *Finder,
                                                           bool Value,
                                                           StringRef Id) {
   Finder->addMatcher(
       compoundStmt(allOf(hasAnySubstatement(ifStmt(hasThen(returnsBool(Value)),
                                                    unless(hasElse(stmt())))),
                          hasAnySubstatement(
                              returnStmt(has(cxxBoolLiteral(equals(!Value))))
-                                 .bind(CompoundReturnId))))
-          .bind(Id),
+                                 .bind(CompoundReturnId)))).bind(Id),
       this);
 }
 
 void SimplifyBooleanExprCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "ChainedConditionalReturn", ChainedConditionalReturn);
   Options.store(Opts, "ChainedConditionalAssignment",
                 ChainedConditionalAssignment);
 }
replaceWithAssignment(Result, IfAssignNot, true);
                  Result.Nodes.getNodeAs<CompoundStmt>(CompoundBoolId)) {
     replaceCompoundReturnWithCondition(Result, Compound);
   } else if (const auto *Compound =
                  Result.Nodes.getNodeAs<CompoundStmt>(CompoundNotBoolId)) {
     replaceCompoundReturnWithCondition(Result, Compound, true);
   }
 }
 
+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 diagnostic = diag(Loc, Description);
+  if (!containsDiscardedTokens(Result, CharRange))
+    diagnostic << FixItHint::CreateReplacement(CharRange, Replacement);
+}
+
 void SimplifyBooleanExprCheck::replaceWithExpression(
     const ast_matchers::MatchFinder::MatchResult &Result,
     const CXXBoolLiteralExpr *BoolLiteral, bool UseLHS, bool Negated) {
   const auto *LHS = Result.Nodes.getNodeAs<Expr>(LHSId);
   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(),
 }
 
 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(),
+  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(
     const MatchFinder::MatchResult &Result, const IfStmt *If, bool Negated) {
   StringRef Terminator = isa<CompoundStmt>(If->getElse()) ? ";" : "";
   std::string Condition = replacementExpression(Result, Negated, If->getCond());
   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(
     const MatchFinder::MatchResult &Result, const CompoundStmt *Compound,
     bool Negated) {
   const auto *Ret = Result.Nodes.getNodeAs<ReturnStmt>(CompoundReturnId);
 
   // The body shouldn't be empty because the matcher ensures that it must
if (const auto *If = dyn_cast<IfStmt>(*Current)) {
       if (const CXXBoolLiteralExpr *Lit = stmtReturnsBool(If, Negated)) {
         if (*After == Ret) {
           if (!ChainedConditionalReturn && BeforeIf)
             continue;
 
           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;
         }
 
         BeforeIf = If;
       }
     } else {
       BeforeIf = nullptr;
     }
   }
 }
 
 void SimplifyBooleanExprCheck::replaceWithAssignment(
     const MatchFinder::MatchResult &Result, const IfStmt *IfAssign,
     bool Negated) {
   SourceRange Range = IfAssign->getSourceRange();
   StringRef VariableName =
       getText(Result, *Result.Nodes.getNodeAs<Expr>(IfAssignVariableId));
   StringRef Terminator = isa<CompoundStmt>(IfAssign->getElse()) ? ";" : "";
   std::string Condition =
       replacementExpression(Result, Negated, IfAssign->getCond());
   std::string Replacement =
       (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
 } // namespace tidy
 } // namespace clang

test/clang-tidy/readability-simplify-bool-expr.cpp

bool negated_null_pointer_condition(int *p4) {
   if (!p4) {
     return false;
   } else {
     return true;
   }
 }
 // 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
+// CHECK-FIXES: {{^}}  if (b) {
+// CHECK-FIXES: // something wicked this way comes{{$}}
+
+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
+// CHECK-FIXES: {{^}}  if (b) {
+// CHECK-FIXES: {{^}}#define SOMETHING_WICKED false