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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
