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

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Thank you for the fix. See the comments inline.


================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:493
@@ +492,3 @@
+    const ast_matchers::MatchFinder::MatchResult &Result, SourceRange Range) {
+  CharSourceRange CharRange = Lexer::makeFileCharRange(
+      CharSourceRange::getTokenRange(Range), *Result.SourceManager,
----------------
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.

================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:497
@@ +496,3 @@
+
+  std::string ReplacementText =
+      Lexer::getSourceText(CharRange, *Result.SourceManager,
----------------
Please use `StringRef` to avoid unnecessary string allocation and copying.

================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:507
@@ +506,3 @@
+  while (!Lex.LexFromRawLexer(Tok)) {
+    if (Tok.is(tok::TokenKind::comment) || Tok.is(tok::TokenKind::hash)) {
+      return true;
----------------
It's more common in this code to omit braces around single-line `if` bodies.

================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:519
@@ +518,3 @@
+    StringRef Replacement) {
+  if (containsDiscardedTokens(Result, ReplacementRange)) {
+    diag(Loc, Description);
----------------

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);
```

================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:533
@@ -499,3 +532,3 @@
       replacementExpression(Result, Negated, UseLHS ? LHS : RHS);
   SourceLocation Start = LHS->getLocStart();
   SourceLocation End = RHS->getLocEnd();
----------------
I'd slightly prefer to create a SourceRange right away:
```
SourceRange Range(LHS->getLocStart(), RHS->getLocEnd());
issueDiag(..., Range, ...);
```

================
Comment at: test/clang-tidy/readability-simplify-bool-expr.cpp:883
@@ +882,3 @@
+}
+// CHECK-MESSAGES: :[[@LINE-6]]:12: warning: {{.*}} in conditional return
+
----------------
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.


http://reviews.llvm.org/D15737