clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[LegalizeAdulthood]

Summary

Expand the simplify boolean expression check to handle implicit conversion of integral types to bool
and improve the handling of implicit conversion of member pointers to bool.

Implicit conversion of member pointers are replaced with explicit comparisons to nullptr.

Implicit conversions of integral types are replaced with explicit comparisons to 0.

[aaron.ballman]

clang-tidy/readability/SimplifyBooleanExprCheck.h

21–22

Can you also update for member pointers?

21–22

To me, this does not improve readability -- I now have to think much harder about operator precedence. Including parens would help significantly, IMO, so that it becomes (i & 1) != 0.

docs/clang-tidy/checks/readability-simplify-boolean-expr.rst

60

Update for member pointers.

[LegalizeAdulthood]

clang-tidy/readability/SimplifyBooleanExprCheck.h

21–22

To clarify: You'd like to see parens around the expression when it is a binary operator, correct?

When it is a variable, there's no need to add parentheses.

21–22

I can explicitly state that; to me "pointers" is a term that includes member pointers.

[aaron.ballman]

clang-tidy/readability/SimplifyBooleanExprCheck.h

21–22

Hmm, perhaps I am being too pedantic. I'm used to thinking of member pointers as different from pointers because it's that way in standardese.

21–22

Correct; if it's just a DeclRefExpr or unary operator then parens aren't useful; but if it's a binary operator, I think parens may help clarify.

[LegalizeAdulthood]

docs/clang-tidy/checks/readability-simplify-boolean-expr.rst

60

Do I really need to explicitly say member pointer as well? It seems redundant.

I didn't update it because a pointer to a member is a pointer. When used as an implicit conversion to bool, the syntax is no different for a plain pointer than for a member pointer. If the syntax were different, I could see your point.

[LegalizeAdulthood]

clang-tidy/readability/SimplifyBooleanExprCheck.h

21–22

Ironically, in another review we were talking about eliminating redundant parentheses globally and these parentheses added for binary operators would be considered redundant and removed.

[aaron.ballman]

clang-tidy/readability/SimplifyBooleanExprCheck.h

21–22

In the other review, I was suggesting that parens in expressions involving binary operators of differing precedence should not be removed. ;-)

docs/clang-tidy/checks/readability-simplify-boolean-expr.rst

60

A pointer to a member is not a pointer according to the language standard, and that's why it may not be a bad idea to call it out explicitly as being supported. From [basic.compound]p3, "Except for pointers to static members, text referring to “pointers” does not apply to pointers to members." I do take your point about the syntax of the implicit conversion being the same, but it would be nice to be explicit about what we support. I would just change it to say "...implicit conversion of a pointer (or pointer to member) to `bool`..."

[LegalizeAdulthood]

docs/clang-tidy/checks/readability-simplify-boolean-expr.rst

60

Meh. I don't mind doing that, but the standard gets hyper-anal about how everything is called because without it the standard would be much more ambiguous. I certainly don't want clang (or clang-tidy) documentation to start reading like the C++ Standard. In other words, while such specific language is necessary in the standard, I don't think anyone is going to be confused by how I originally worded it.

[LegalizeAdulthood]

Update from review comments.

[aaron.ballman]

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

192

Can we add a test to ensure that we aren't suggesting nullptr for a C program? Perhaps we could simply substitute nullptr for >= C++11 and NULL for everything else?

clang-tidy/readability/SimplifyBooleanExprCheck.h

21–22

Perhaps a different idea regarding parens isn't to add/remove parens in various checks for readability, but instead have two readability checks that do different things (both disabled by default?): (1) remove spurious parens where the presence of the parens has no effect on the order of evaluation of the subexpressions, and (2) add parens where there is an operator precedence difference between the operators of two subexpressions. Both of these checks are at odds with one another (which is why I don't think it makes sense to enable them by default), but both certainly seem like they would be useful.

Thoughts on the general idea (not trying to sign either of us up for work to implement it)?

docs/clang-tidy/checks/readability-simplify-boolean-expr.rst

60

I guess I view it differently; this isn't being hyper-anal, it's precisely stating what we support so that the user doesn't have to guess. However, I'm also fine leaving it out because it isn't likely to cause confusion for too long -- a simple test gives the answer if anyone is wondering.

[LegalizeAdulthood]

clang-tidy/readability/SimplifyBooleanExprCheck.h

21–22

I'm of two minds on this, I'm not really coming down hard on either side. It feels like there should be a readability check for removing parenthesis to consolidate the heuristics all in one place and that check can in turn be configured by parameters.

[LegalizeAdulthood]

Update from review comments.
trunk clang-format

[LegalizeAdulthood]

Inline Variable

[LegalizeAdulthood]

Move local function to anonymous namespace

[aaron.ballman]

clang-tidy/readability/SimplifyBooleanExprCheck.h

21–23

I think I can agree with that, but I also think it depends a lot on what the purpose to the check is. If it's "improve readability regarding parens" + options that control when to remove or add, that makes sense to me. Otherwise, I think segregating the checks into one that removes (plus options) and one that adds (plus options) makes sense -- even if they perhaps use the same underlying heuristic engine and are simply surfaced as separate checks.

[LegalizeAdulthood]

clang-tidy/readability/SimplifyBooleanExprCheck.h

21–23

This check isn't at all about the readability of parens, so it doesn't make sense for this check to be concerned with it IMO.

[aaron.ballman]

clang-tidy/readability/SimplifyBooleanExprCheck.h

21–23

Agreed; trying to suss out what the appropriate design for this particular check is. I think I've talked myself into "don't touch parens here."

[LegalizeAdulthood]

clang-tidy/readability/SimplifyBooleanExprCheck.h

21–23

Currently in this patch, parens are added when the expression compared to nullptr or 0 is a binary operator.

I think that is the right thing to do here so we get:

bool b = (i & 1) == 0;

instead of

bool b = i & 1 == 0;

[aaron.ballman]

clang-tidy/readability/SimplifyBooleanExprCheck.h

21–23

And we've come full circle to my original suggestion. :-D Personally, I find it to be more readable with the parens than without. I'm just worried that we have this check adding parens and another check removing them. But that can be handled in the check that removes them instead of here. My statement about not touching parens was horribly unclear, and I am sorry for that. What my brain was telling my fingers to type was: Don't worry about parens here; add them if they add clarity, don't add them if they don't add clarity. We can worry about parens categorically as part of another check.

[LegalizeAdulthood]

clang-tidy/readability/SimplifyBooleanExprCheck.h

21–23

Yes, I did update the patch per your earlier review comments.

I agree this is more readable and that a future check that generally removes unnecessary parens can handle it if people don't like it.

Because there is no "right" answer, I prefer to deploy the check and await feedback from users to know if adding the parens is widely considered bad. If the general consensus is that adding the parens is bad, this check can be enhanced with an option to decide when to add them. Prior to this change, it only added them when necessary to preserve semantics.

In trying this check on llvm code base I have seen one occasion where the check produced something like !!(complicated expression), but I haven't figured that one out yet. It's on my list of things to improve with this check.

Once this change is accepted, I have another improvement waiting in the wings to simplify things like !(a < b) to (a >= b), etc. There has been some discussion earlier about how negated combinations of boolean expressions should be handled. The majority of the comments so far feel that applying DeMorgan's law generally makes for better readability, but again with readability checks not everyone agrees.

For now, I believe I have addressed all of Aaron's outstanding comments.

[aaron.ballman]

LGTM!

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

368

Sorry for not noticing this earlier, but since we have two other in-flight patches I reviewed this morning, it caught my attention: we should not be using isExpansionInMainFile, but instead testing the source location of the matches to see if they're in a macro expansion. This is usually done with something like Result.SourceManager->isMacroBodyExpansion(SomeLoc).

I realize this is existing code and not your problem, but it should be fixed in a follow-on patch (by someone) and include some macro test cases.

[LegalizeAdulthood]

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

368

There is an open bug on this that I will address. And while this is existing code, I am the author of the original check :).

I also experienced a problem when running this check on some code that did something like:

#define FOO (par[1] > 4)

// ...
bool f() {
  if (!FOO) {
    return true;
  } else {
    return false;
  }
}

So it needs improvement w.r.t. macros and that is also on my to-do list.

I'm trying to do things in incremental steps and not giant changes.

[LegalizeAdulthood]

I do not have commit access. If someone could commit this for me, I would appreciate it.

Patch by Richard Thomson.

Thanks.

[LegalizeAdulthood]

It's been almost a week. Is there some reason this hasn't been committed yet?

[aaron.ballman]

Commit in r260096, sorry for the delay. You should speak to Chris Lattner about getting commit privileges so that you're not blocked on one of us being available to commit on your behalf again.

[aaron.ballman]

This is causing build bot failures:

http://bb.pgr.jp/builders/cmake-clang-tools-x86_64-linux/builds/23351
http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/492
http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/812

The failure is not obvious to me, and I don't have the opportunity to debug locally right now, so I have reverted in r260097.

[LegalizeAdulthood]

In article <[e-mail]>,

Aaron Ballman <[e-mail]> writes:

aaron.ballman added a comment.

This is causing build bot failures:

http://bb.pgr.jp/builders/cmake-clang-tools-x86_64-linux/builds/23351
http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/492
http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/812

The failure is not obvious to me, and I don't have the opportunity to debug
locally right now, so I have reverted in r260097.

make check-all passes for me. I don't see what the problem is in the logs
either.

[LegalizeAdulthood]

Fixes StringRef problem that crashes tests in release builds only.

[LegalizeAdulthood]

If someone could review the changes and commit this corrected version for me, that would be great.

Patch by Richard Thomson.

[aaron.ballman]

I have commit in r260681. You should look into obtaining commit privileges: http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

~Aaron

clang-tidy/readability/SimplifyBooleanExprCheck.h

 #include "../ClangTidy.h"
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 /// Looks for boolean expressions involving boolean constants and simplifies
 /// them to use the appropriate boolean expression directly.
 ///
-/// Examples:
-///
-/// ===========================================  ================
-/// Initial expression                           Result
-/// -------------------------------------------  ----------------
-/// `if (b == true)`                             `if (b)`
-/// `if (b == false)`                            `if (!b)`
-/// `if (b && true)`                             `if (b)`
-/// `if (b && false)`                            `if (false)`
-/// `if (b || true)`                             `if (true)`
-/// `if (b || false)`                            `if (b)`
-/// `e ? true : false`                           `e`
-/// `e ? false : true`                           `!e`
-/// `if (true) t(); else f();`                   `t();`
-/// `if (false) t(); else f();`                  `f();`
-/// `if (e) return true; else return false;`     `return e;`
-/// `if (e) return false; else return true;`     `return !e;`
-/// `if (e) b = true; else b = false;`           `b = e;`
-/// `if (e) b = false; else b = true;`           `b = !e;`
-/// `if (e) return true; return false;`          `return e;`
-/// `if (e) return false; return true;`          `return !e;`
-/// ===========================================  ================
-///
-/// The resulting expression `e` is modified as follows:
-///   1. Unnecessary parentheses around the expression are removed.
-///   2. Negated applications of `!` are eliminated.
-///   3. Negated applications of comparison operators are changed to use the
-///      opposite condition.
-///   4. Implicit conversions of pointer to `bool` are replaced with explicit
-///      comparisons to `nullptr`.
-///   5. Implicit casts to `bool` are replaced with explicit casts to `bool`.
-///   6. Object expressions with `explicit operator bool` conversion operators
-///      are replaced with explicit casts to `bool`.
-///
-/// Examples:
-///   1. The ternary assignment `bool b = (i < 0) ? true : false;` has redundant
-///      parentheses and becomes `bool b = i < 0;`.
-///
-///   2. The conditional return `if (!b) return false; return true;` has an
-///      implied double negation and becomes `return b;`.
-///
-///   3. The conditional return `if (i < 0) return false; return true;` becomes
-///      `return i >= 0;`.
-///
-///      The conditional return `if (i != 0) return false; return true;` becomes
-///      `return i == 0;`.
-///
-///   4. The conditional return `if (p) return true; return false;` has an
-///      implicit conversion of a pointer to `bool` and becomes
-///      `return p != nullptr;`.
-///
-///      The ternary assignment `bool b = (i & 1) ? true : false;` has an
-///      implicit conversion of `i & 1` to `bool` and becomes
-///      `bool b = static_cast<bool>(i & 1);`.
-///
-///   5. The conditional return `if (i & 1) return true; else return false;` has
-///      an implicit conversion of an integer quantity `i & 1` to `bool` and
-///      becomes `return static_cast<bool>(i & 1);`
-///
-///   6. Given `struct X { explicit operator bool(); };`, and an instance `x` of
-///      `struct X`, the conditional return `if (x) return true; return false;`
-///      becomes `return static_cast<bool>(x);`
-///
-/// When a conditional boolean return or assignment appears at the end of a
-/// chain of `if`, `else if` statements, the conditional statement is left
-/// unchanged unless the option `ChainedConditionalReturn` or
-/// `ChainedConditionalAssignment`, respectively, is specified as non-zero.
-/// The default value for both options is zero.
-///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-simplify-boolean-expr.html
 class SimplifyBooleanExprCheck : public ClangTidyCheck {
 public:
   SimplifyBooleanExprCheck(StringRef Name, ClangTidyContext *Context);
 
   void storeOptions(ClangTidyOptions::OptionMap &Options) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

internal::Matcher<Stmt> returnsBool(bool Value, StringRef Id = "ignored") {
   return anyOf(SimpleReturnsBool,
                compoundStmt(statementCountIs(1), has(SimpleReturnsBool)));
 }
 
 bool needsParensAfterUnaryNegation(const Expr *E) {
   E = E->IgnoreImpCasts();
   if (isa<BinaryOperator>(E) || isa<ConditionalOperator>(E))
     return true;
-  if (const auto *Op = dyn_cast<CXXOperatorCallExpr>(E)) {
+
+  if (const auto *Op = dyn_cast<CXXOperatorCallExpr>(E))
     return Op->getNumArgs() == 2 && Op->getOperator() != OO_Call &&
            Op->getOperator() != OO_Subscript;
-  }
+
   return false;
 }
 
 std::pair<BinaryOperatorKind, BinaryOperatorKind> Opposites[] = {
     {BO_LT, BO_GE}, {BO_GT, BO_LE}, {BO_EQ, BO_NE}};
 
 StringRef negatedOperator(const BinaryOperator *BinOp) {
   const BinaryOperatorKind Opcode = BinOp->getOpcode();
   for (auto NegatableOp : Opposites) {
     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();
std::string asBool(StringRef text, bool NeedsStaticCast) {
   if (NeedsStaticCast)
     return ("static_cast<bool>(" + text + ")").str();
 
   return text;
 }
 
 bool needsNullPtrComparison(const Expr *E) {
   if (const auto *ImpCast = dyn_cast<ImplicitCastExpr>(E))
-    return ImpCast->getCastKind() == CK_PointerToBoolean;
+    return ImpCast->getCastKind() == CK_PointerToBoolean ||
+           ImpCast->getCastKind() == CK_MemberPointerToBoolean;
+
+  return false;
+}
+
+bool needsZeroComparison(const Expr *E) {
+  if (const auto *ImpCast = dyn_cast<ImplicitCastExpr>(E))
+    return ImpCast->getCastKind() == CK_IntegralToBoolean;
 
   return false;
 }
 
 bool needsStaticCast(const Expr *E) {
   if (const auto *ImpCast = dyn_cast<ImplicitCastExpr>(E)) {
     if (ImpCast->getCastKind() == CK_UserDefinedConversion &&
         ImpCast->getSubExpr()->getType()->isBooleanType()) {
       if (const auto *MemCall =
               dyn_cast<CXXMemberCallExpr>(ImpCast->getSubExpr())) {
         if (const auto *MemDecl =
                 dyn_cast<CXXConversionDecl>(MemCall->getMethodDecl())) {
           if (MemDecl->isExplicit())
             return true;
         }
       }
     }
   }
 
   E = E->IgnoreImpCasts();
   return !E->getType()->isBooleanType();
 }
 
+std::string compareExpressionToConstant(const MatchFinder::MatchResult &Result,
+                                        const Expr *E, bool Negated,
+                                        const char *Constant) {
+  E = E->IgnoreImpCasts();
+  const std::string ExprText =
+      (isa<BinaryOperator>(E) ? ("(" + getText(Result, *E) + ")")
+                              : getText(Result, *E))
+          .str();
+  return ExprText + " " + (Negated ? "!=" : "==") + " " + Constant;
+}
+
+std::string compareExpressionToNullPtr(const MatchFinder::MatchResult &Result,
+                                       const Expr *E, bool Negated) {
+  const char *NullPtr =
+      Result.Context->getLangOpts().CPlusPlus11 ? "nullptr" : "NULL";
+  return compareExpressionToConstant(Result, E, Negated, NullPtr);
+}
+
+std::string compareExpressionToZero(const MatchFinder::MatchResult &Result,
+                                    const Expr *E, bool Negated) {
+  return compareExpressionToConstant(Result, E, Negated, "0");
+}
+
 std::string replacementExpression(const MatchFinder::MatchResult &Result,
                                   bool Negated, const Expr *E) {
   E = E->ignoreParenBaseCasts();
   const bool NeedsStaticCast = needsStaticCast(E);
   if (Negated) {
     if (const auto *UnOp = dyn_cast<UnaryOperator>(E)) {
       if (UnOp->getOpcode() == UO_LNot) {
         if (needsNullPtrComparison(UnOp->getSubExpr()))
-          return (getText(Result, *UnOp->getSubExpr()) + " != nullptr").str();
+          return compareExpressionToNullPtr(Result, UnOp->getSubExpr(), true);
+
+        if (needsZeroComparison(UnOp->getSubExpr()))
+          return compareExpressionToZero(Result, UnOp->getSubExpr(), true);
 
         return replacementExpression(Result, false, UnOp->getSubExpr());
       }
     }
 
     if (needsNullPtrComparison(E))
-      return (getText(Result, *E) + " == nullptr").str();
+      return compareExpressionToNullPtr(Result, E, false);
+
+    if (needsZeroComparison(E))
+      return compareExpressionToZero(Result, E, false);
 
     StringRef NegatedOperator;
     const Expr *LHS = nullptr;
     const Expr *RHS = nullptr;
     if (const auto *BinOp = dyn_cast<BinaryOperator>(E)) {
       NegatedOperator = negatedOperator(BinOp);
       LHS = BinOp->getLHS();
       RHS = BinOp->getRHS();
     } else if (const auto *OpExpr = dyn_cast<CXXOperatorCallExpr>(E)) {
       if (OpExpr->getNumArgs() == 2) {
         NegatedOperator = negatedOperator(OpExpr);
         LHS = OpExpr->getArg(0);
         RHS = OpExpr->getArg(1);
       }
     }
-    if (!NegatedOperator.empty() && LHS && RHS) {
+    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))
-      return (getText(Result, *E) + " == nullptr").str();
+      return compareExpressionToNullPtr(Result, E, false);
+
+    if (needsZeroComparison(E))
+      return compareExpressionToZero(Result, E, false);
 
     return ("!" + asBool(Text, NeedsStaticCast));
   }
 
   if (const auto *UnOp = dyn_cast<UnaryOperator>(E)) {
     if (UnOp->getOpcode() == UO_LNot) {
       if (needsNullPtrComparison(UnOp->getSubExpr()))
-        return (getText(Result, *UnOp->getSubExpr()) + " == nullptr").str();
+        return compareExpressionToNullPtr(Result, UnOp->getSubExpr(), false);
+
+      if (needsZeroComparison(UnOp->getSubExpr()))
+        return compareExpressionToZero(Result, UnOp->getSubExpr(), false);
     }
   }
 
   if (needsNullPtrComparison(E))
-    return (getText(Result, *E) + " != nullptr").str();
+    return compareExpressionToNullPtr(Result, E, true);
+
+  if (needsZeroComparison(E))
+    return compareExpressionToZero(Result, E, true);
 
   return asBool(getText(Result, *E), NeedsStaticCast);
 }
 
 const CXXBoolLiteralExpr *stmtReturnsBool(const ReturnStmt *Ret, bool Negated) {
   if (const auto *Bool = dyn_cast<CXXBoolLiteralExpr>(Ret->getRetValue())) {
     if (Bool->getValue() == !Negated)
       return Bool;
if (Compound->size() == 1) {
       if (const auto *CompoundRet = dyn_cast<ReturnStmt>(Compound->body_back()))
         return stmtReturnsBool(CompoundRet, Negated);
     }
   }
 
   return nullptr;
 }
 
+bool containsDiscardedTokens(const MatchFinder::MatchResult &Result,
+                             CharSourceRange CharRange) {
+  std::string 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;
+}
+
 } // namespace
 
 SimplifyBooleanExprCheck::SimplifyBooleanExprCheck(StringRef Name,
                                                    ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       ChainedConditionalReturn(Options.get("ChainedConditionalReturn", 0U)),
       ChainedConditionalAssignment(
           Options.get("ChainedConditionalAssignment", 0U)) {}
Finder->addMatcher(
       this);
 }
 
 void SimplifyBooleanExprCheck::matchBoolCompOpExpr(MatchFinder *Finder,
                                                    bool Value,
                                                    StringRef OperatorName,
                                                    StringRef BooleanId) {
   Finder->addMatcher(
-      binaryOperator(isExpansionInMainFile(), hasOperatorName(OperatorName),
-                     hasLHS(allOf(expr().bind(LHSId),
-                                  ignoringImpCasts(cxxBoolLiteral(equals(Value))
-                                                       .bind(BooleanId)))),
-                     hasRHS(expr().bind(RHSId)),
-                     unless(hasRHS(hasDescendant(cxxBoolLiteral())))),
+      binaryOperator(
+          isExpansionInMainFile(), hasOperatorName(OperatorName),
+          hasLHS(allOf(
+              expr().bind(LHSId),
+              ignoringImpCasts(cxxBoolLiteral(equals(Value)).bind(BooleanId)))),
       this);
 }
 
 void SimplifyBooleanExprCheck::matchExprCompOpBool(MatchFinder *Finder,
                                                    bool Value,
                                                    StringRef OperatorName,
                                                    StringRef BooleanId) {
   Finder->addMatcher(
-      binaryOperator(isExpansionInMainFile(), hasOperatorName(OperatorName),
-                     unless(hasLHS(hasDescendant(cxxBoolLiteral()))),
-                     hasLHS(expr().bind(LHSId)),
-                     hasRHS(allOf(expr().bind(RHSId),
-                                  ignoringImpCasts(cxxBoolLiteral(equals(Value))
-                                                       .bind(BooleanId))))),
+      binaryOperator(
+          isExpansionInMainFile(), hasOperatorName(OperatorName),
+                       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),
-                     this);
+  Finder->addMatcher(
+      ifStmt(isExpansionInMainFile(),
+             hasCondition(cxxBoolLiteral(equals(Value)).bind(BooleanId)))
+          .bind(IfStmtId),
 }
 
 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) {
+  if (ChainedConditionalReturn)
     Finder->addMatcher(ifStmt(isExpansionInMainFile(),
                               hasThen(returnsBool(Value, ThenLiteralId)),
-                              hasElse(returnsBool(!Value))).bind(Id),
+                              hasElse(returnsBool(!Value)))
+                           .bind(Id),
                        this);
-  } else {
+  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("="),
       hasLHS(declRefExpr(hasDeclaration(decl().bind(IfAssignObjId)))),
       hasLHS(expr().bind(IfAssignVariableId)),
       hasRHS(cxxBoolLiteral(equals(Value)).bind(IfAssignLocId)));
   auto Then = anyOf(SimpleThen, compoundStmt(statementCountIs(1),
                                              hasAnySubstatement(SimpleThen)));
   auto SimpleElse = binaryOperator(
       hasOperatorName("="),
       hasLHS(declRefExpr(hasDeclaration(equalsBoundNode(IfAssignObjId)))),
       hasRHS(cxxBoolLiteral(equals(!Value))));
   auto Else = anyOf(SimpleElse, compoundStmt(statementCountIs(1),
                                              hasAnySubstatement(SimpleElse)));
-  if (ChainedConditionalAssignment) {
+  if (ChainedConditionalAssignment)
     Finder->addMatcher(
         ifStmt(isExpansionInMainFile(), hasThen(Then), hasElse(Else)).bind(Id),
         this);
-  } else {
+  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);
 }
void SimplifyBooleanExprCheck::registerMatchers(MatchFinder *Finder) {
   matchIfAssignsBool(Finder, false, IfAssignNotBoolId);
 
   matchCompoundIfReturnsBool(Finder, true, CompoundBoolId);
   matchCompoundIfReturnsBool(Finder, false, CompoundNotBoolId);
 }
 
 void SimplifyBooleanExprCheck::check(const MatchFinder::MatchResult &Result) {
   if (const CXXBoolLiteralExpr *LeftRemoved =
-          getBoolLiteral(Result, RightExpressionId)) {
+          getBoolLiteral(Result, RightExpressionId))
     replaceWithExpression(Result, LeftRemoved, false);
-  } else if (const CXXBoolLiteralExpr *RightRemoved =
-                 getBoolLiteral(Result, LeftExpressionId)) {
+  else if (const CXXBoolLiteralExpr *RightRemoved =
+               getBoolLiteral(Result, LeftExpressionId))
     replaceWithExpression(Result, RightRemoved, true);
-  } else if (const CXXBoolLiteralExpr *NegatedLeftRemoved =
-                 getBoolLiteral(Result, NegatedRightExpressionId)) {
+  else if (const CXXBoolLiteralExpr *NegatedLeftRemoved =
+               getBoolLiteral(Result, NegatedRightExpressionId))
     replaceWithExpression(Result, NegatedLeftRemoved, false, true);
-  } else if (const CXXBoolLiteralExpr *NegatedRightRemoved =
-                 getBoolLiteral(Result, NegatedLeftExpressionId)) {
+  else if (const CXXBoolLiteralExpr *NegatedRightRemoved =
+               getBoolLiteral(Result, NegatedLeftExpressionId))
     replaceWithExpression(Result, NegatedRightRemoved, true, true);
-  } else if (const CXXBoolLiteralExpr *TrueConditionRemoved =
-                 getBoolLiteral(Result, ConditionThenStmtId)) {
+  else if (const CXXBoolLiteralExpr *TrueConditionRemoved =
+               getBoolLiteral(Result, ConditionThenStmtId))
     replaceWithThenStatement(Result, TrueConditionRemoved);
-  } else if (const CXXBoolLiteralExpr *FalseConditionRemoved =
-                 getBoolLiteral(Result, ConditionElseStmtId)) {
+  else if (const CXXBoolLiteralExpr *FalseConditionRemoved =
+               getBoolLiteral(Result, ConditionElseStmtId))
     replaceWithElseStatement(Result, FalseConditionRemoved);
-  } else if (const auto *Ternary =
-                 Result.Nodes.getNodeAs<ConditionalOperator>(TernaryId)) {
+  else if (const auto *Ternary =
+               Result.Nodes.getNodeAs<ConditionalOperator>(TernaryId))
     replaceWithCondition(Result, Ternary);
-  } else if (const auto *TernaryNegated =
-                 Result.Nodes.getNodeAs<ConditionalOperator>(
-                     TernaryNegatedId)) {
+  else if (const auto *TernaryNegated =
+               Result.Nodes.getNodeAs<ConditionalOperator>(TernaryNegatedId))
     replaceWithCondition(Result, TernaryNegated, true);
-  } else if (const auto *If = Result.Nodes.getNodeAs<IfStmt>(IfReturnsBoolId)) {
+  else if (const auto *If = Result.Nodes.getNodeAs<IfStmt>(IfReturnsBoolId))
     replaceWithReturnCondition(Result, If);
-  } else if (const auto *IfNot =
-                 Result.Nodes.getNodeAs<IfStmt>(IfReturnsNotBoolId)) {
+  else if (const auto *IfNot =
+               Result.Nodes.getNodeAs<IfStmt>(IfReturnsNotBoolId))
     replaceWithReturnCondition(Result, IfNot, true);
-  } else if (const auto *IfAssign =
-                 Result.Nodes.getNodeAs<IfStmt>(IfAssignBoolId)) {
+  else if (const auto *IfAssign =
+               Result.Nodes.getNodeAs<IfStmt>(IfAssignBoolId))
     replaceWithAssignment(Result, IfAssign);
-  } else if (const auto *IfAssignNot =
-                 Result.Nodes.getNodeAs<IfStmt>(IfAssignNotBoolId)) {
+  else if (const auto *IfAssignNot =
+               Result.Nodes.getNodeAs<IfStmt>(IfAssignNotBoolId))
     replaceWithAssignment(Result, IfAssignNot, true);
-  } else if (const auto *Compound =
-                 Result.Nodes.getNodeAs<CompoundStmt>(CompoundBoolId)) {
+  else if (const auto *Compound =
+               Result.Nodes.getNodeAs<CompoundStmt>(CompoundBoolId))
     replaceCompoundReturnWithCondition(Result, Compound);
-  } else if (const auto *Compound =
-                 Result.Nodes.getNodeAs<CompoundStmt>(CompoundNotBoolId)) {
+  else if (const auto *Compound =
+               Result.Nodes.getNodeAs<CompoundStmt>(CompoundNotBoolId))
     replaceCompoundReturnWithCondition(Result, Compound, true);
-  }
-}
-
-bool containsDiscardedTokens(
-    const ast_matchers::MatchFinder::MatchResult &Result,
-    CharSourceRange CharRange) {
-  std::string 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());

 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
   // contain at least two statements:
   // 1) A `return` statement returning a boolean literal `false` or `true`
-  // 2) An `if` statement with no `else` clause that consists fo a single
+  // 2) An `if` statement with no `else` clause that consists of a single
   //    `return` statement returning the opposite boolean literal `true` or
   //    `false`.
   assert(Compound->size() >= 2);
   const IfStmt *BeforeIf = nullptr;
   CompoundStmt::const_body_iterator Current = Compound->body_begin();
   CompoundStmt::const_body_iterator After = Compound->body_begin();
   for (++After; After != Compound->body_end() && *Current != Ret;
        ++Current, ++After) {

docs/clang-tidy/checks/readability-simplify-boolean-expr.rst

 ``if (e) return false; return true;``          ``return !e;``
 ===========================================  ================
 
 The resulting expression ``e`` is modified as follows:
   1. Unnecessary parentheses around the expression are removed.
   2. Negated applications of ``!`` are eliminated.
   3. Negated applications of comparison operators are changed to use the
      opposite condition.
-  4. Implicit conversions of pointer to ``bool`` are replaced with explicit
-     comparisons to ``nullptr``.
+  4. Implicit conversions of pointers, including pointers to members, to
+     ``bool`` are replaced with explicit comparisons to ``nullptr`` in C++11
+      or ``NULL`` in C++98/03.
   5. Implicit casts to ``bool`` are replaced with explicit casts to ``bool``.
   6. Object expressions with ``explicit operator bool`` conversion operators
      are replaced with explicit casts to ``bool``.
+  7. Implicit conversions of integral types to ``bool`` are replaced with
+     explicit comparisons to ``0``.
 
 Examples:
   1. The ternary assignment ``bool b = (i < 0) ? true : false;`` has redundant
      parentheses and becomes ``bool b = i < 0;``.
 
   2. The conditional return ``if (!b) return false; return true;`` has an
      implied double negation and becomes ``return b;``.
 
   3. The conditional return ``if (i < 0) return false; return true;`` becomes
      ``return i >= 0;``.
 
      The conditional return ``if (i != 0) return false; return true;`` becomes
      ``return i == 0;``.
 
   4. The conditional return ``if (p) return true; return false;`` has an
      implicit conversion of a pointer to ``bool`` and becomes
      ``return p != nullptr;``.
 
      The ternary assignment ``bool b = (i & 1) ? true : false;`` has an
      implicit conversion of ``i & 1`` to ``bool`` and becomes
-     ``bool b = static_cast<bool>(i & 1);``.
+     ``bool b = (i & 1) != 0;``.
 
   5. The conditional return ``if (i & 1) return true; else return false;`` has
      an implicit conversion of an integer quantity ``i & 1`` to ``bool`` and
-     becomes ``return static_cast<bool>(i & 1);``
+     becomes ``return (i & 1) != 0;``
 
   6. Given ``struct X { explicit operator bool(); };``, and an instance ``x`` of
      ``struct X``, the conditional return ``if (x) return true; return false;``
      becomes ``return static_cast<bool>(x);``
 
 When a conditional boolean return or assignment appears at the end of a
 chain of ``if``, ``else if`` statements, the conditional statement is left
 unchanged unless the option ``ChainedConditionalReturn`` or
 ``ChainedConditionalAssignment``, respectively, is specified as non-zero.
 The default value for both options is zero.
 

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

 bool if_implicit_bool_expr(int i) {
   if (i & 1) {
     return true;
   } else {
     return false;
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
-// CHECK-FIXES: {{^}}  return static_cast<bool>(i & 1);{{$}}
+// CHECK-FIXES: {{^}}  return (i & 1) != 0;{{$}}
 
 bool negated_if_implicit_bool_expr(int i) {
   if (i - 1) {
     return false;
   } else {
     return true;
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
-// CHECK-FIXES: {{^}}  return !static_cast<bool>(i - 1);{{$}}
+// CHECK-FIXES: {{^}}  return (i - 1) == 0;{{$}}
 
 bool implicit_int(int i) {
   if (i) {
     return true;
   } else {
     return false;
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
-// CHECK-FIXES: {{^}}  return static_cast<bool>(i);{{$}}
+// CHECK-FIXES: {{^}}  return i != 0;{{$}}
 
 bool explicit_bool(bool b) {
   if (b) {
     return true;
   } else {
     return false;
   }
 }

 bool bitwise_complement_conversion(int i) {
   if (~i) {
     return true;
   } else {
     return false;
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
-// CHECK-FIXES: {{^}}  return static_cast<bool>(~i);{{$}}
+// CHECK-FIXES: {{^}}  return ~i != 0;{{$}}
 
 bool logical_or(bool a, bool b) {
   if (a || b) {
     return true;
   } else {
     return false;
   }
 }

 // CHECK-MESSAGES: :[[@LINE-5]]:9: warning: {{.*}} in conditional assignment
 // CHECK-FIXES: {{^  bool y;$}}
 // CHECK-FIXES: {{^}}  y = static_cast<bool>(x);{{$}}
 
 void ternary_integer_condition(int i) {
   bool b = i ? true : false;
 }
 // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: {{.*}} in ternary expression result
-// CHECK-FIXES: bool b = static_cast<bool>(i);{{$}}
+// CHECK-FIXES: bool b = i != 0;{{$}}
 
 bool non_null_pointer_condition(int *p1) {
   if (p1) {
     return true;
   } else {
     return false;
   }
 }
bool preprocessor_in_the_middle(bool b) {
   } 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
+
+bool integer_not_zero(int i) {
+  if (i) {
+    return false;
+  } else {
+    return true;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}  return i == 0;{{$}}
+
+class A {
+public:
+    int m;
+};
+
+bool member_pointer_nullptr(int A::*p) {
+  if (p) {
+    return true;
+  } else {
+    return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return p != nullptr;{{$}}
+
+bool integer_member_implicit_cast(A *p) {
+  if (p->m) {
+    return true;
+  } else {
+    return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return p->m != 0;{{$}}