Page MenuHomePhabricator

[clang-tidy] SimplifyBoolenExpr doesn't add parens if unary negotiation is of ExprWithCleanups type
ClosedPublic

Authored by zinovy.nis on May 20 2018, 2:03 PM.

Details

Summary
bool foo(A &S) {
  if (S != (A)S)
    return false;
  return true;
}

is fixed into (w/o this patch)

...
return !S != (A)S; // negotiation affects first operand only
}

instead of (with this patch)

...
return S == (A)S; // note == instead of !=
}

Diff Detail

Repository
rL LLVM

Event Timeline

zinovy.nis created this revision.May 20 2018, 2:03 PM
aaron.ballman accepted this revision.May 21 2018, 7:45 AM

LGTM, with a small nit.

clang-tidy/readability/SimplifyBooleanExprCheck.cpp
78 ↗(On Diff #147722)

const auto *, please.

This revision is now accepted and ready to land.May 21 2018, 7:45 AM
zinovy.nis edited the summary of this revision. (Show Details)
  • More accurate place to fix.
zinovy.nis marked an inline comment as done.May 21 2018, 12:37 PM
This revision was automatically updated to reflect the committed changes.
malcolm.parsons added inline comments.
clang-tools-extra/trunk/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
198

E->IgnoreImplicit() can be used to ignore ExprWithCleanups

zinovy.nis added inline comments.May 22 2018, 12:34 PM
clang-tools-extra/trunk/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
198

Thanks. But it seems to be too agressive:

return (i & 1) != 0;

becomes

return static_cast<bool>(i & 1);
zinovy.nis added inline comments.May 22 2018, 1:10 PM
clang-tools-extra/trunk/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
198
if (!isa<ImplicitCastExpr>(E))
  E = E->IgnoreImplicit();

works properly but looks a bit verbose. What do you think?

clang-tools-extra/trunk/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
198

I think what you've committed is fine.