This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by LegalizeAdulthood on Dec 22 2015, 8:48 PM.

Details

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

Diff Detail

Event Timeline

LegalizeAdulthood retitled this revision from to [clang-tidy] Preserve comments and preprocessor directives when simplifying boolean expressions.
LegalizeAdulthood updated this object.
LegalizeAdulthood added a reviewer: alexfh.
LegalizeAdulthood added a subscriber: cfe-commits.
alexfh edited edge metadata.Dec 23 2015, 6:49 AM

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 edited edge metadata.
LegalizeAdulthood marked 6 inline comments as done.

Update from review comments

alexfh accepted this revision.Dec 28 2015, 4:39 AM
alexfh edited edge metadata.

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).

This revision is now accepted and ready to land.Dec 28 2015, 4:39 AM

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.

This revision was automatically updated to reflect the committed changes.