This is an archive of the discontinued LLVM Phabricator instance.

Add readability-simplify-boolean-expr check to clang-tidy
ClosedPublic

Authored by LegalizeAdulthood on Feb 14 2015, 4:03 PM.

Details

Reviewers
alexfh
sbenza
Summary

This check looks for comparisons between boolean expressions and boolean constants and simplifies them to just use the appropriate boolean expression directly.

if (b == true) becomes if (b)
if (b == false) becomes if (!b)
if (b && true) becomes if (b)
if (b && false) becomes if (false)
if (b || true) becomes if (true)
if (b || false) becomes if (b)
e ? true : false becomes e
e ? false : true becomes !e
if (true) t(); else f(); becomes t();
if (false) t(); else f(); becomes f();
if (e) return true; else return false; becomes return (e);
if (e) return false; else return true; becomes return !(e);
if (e) b = true; else b = false; becomes b = e;
if (e) b = false; else b = true; becomes b = !(e);

Diff Detail

Event Timeline

LegalizeAdulthood retitled this revision from to Add readability-simplify-boolean-expr check to clang-tidy.
LegalizeAdulthood updated this object.
LegalizeAdulthood edited the test plan for this revision. (Show Details)
LegalizeAdulthood added a reviewer: alexfh.
LegalizeAdulthood added a subscriber: Unknown Object (MLST).

Hmm... seems like it has a problem with an expression like false || (true && false). The two generated edits for the same line trample on each other.

Richard,

I think this check should allow expressions originating from macros:

#define HAS_XYZ_FEATURE true
static /*constexpr*/ bool returnTrue() { return true; }
if (returnTrue() && HAS_XYZ_FEATURE);

This corrects the erroneous double edit on a single line that creates invalid code. It is less aggressive in simplifying the boolean expression, but prevents invalid fixits.

Richard,

I think this check should allow expressions originating from macros:

#define HAS_XYZ_FEATURE true
static /*constexpr*/ bool returnTrue() { return true; }
if (returnTrue() && HAS_XYZ_FEATURE);

I added these test cases:

static constexpr bool truthy()
{
    return true;
}

#define HAS_XYZ_FEATURE true

void macros_and_constexprs()
{
    int i = 0;
    bool b = (i == 0);
    if (b && truthy()) {
        i = 1;
    }
    i = 2;
    if (b && HAS_XYZ_FEATURE) {
        i = 3;
    }
    i = 4;
}

The second test case was automatically handled by the existing code:

llvm/tools/clang/tools/extra/test/clang-tidy/readability-simplify-bool-expr.cpp:297:14: warning: Redundant boolean constant supplied to boolean operator. [readability-simplify-boolean-expr]
    if (b && HAS_XYZ_FEATURE) {
             ^
        b
llvm/tools/clang/tools/extra/test/clang-tidy/readability-simplify-bool-expr.cpp:287:25: note: expanded from macro 'HAS_XYZ_FEATURE'
#define HAS_XYZ_FEATURE true
                        ^

I will look into the constexpr case.

Add some test cases for macros and constexpr functions.
Re-enable some temporarily commented out checking on other test cases.

I looked at the constexpr case and while it could be done, it's quite a bit of work for what feels like an edge condition.

The primary intent of this check is to look for cases where we used boolean expressions in conjunction with boolean literals. The extra literals are simply redundant.

However, with constexpr functions it isn't so cut-and-dried. It seems that if you have a constexpr function that is hardcoded to always return true or false, that the better fixup would be to Inline Function on those constexpr functions. At that point you could run this existing simplification and the usage would be detected and simplified.

LegalizeAdulthood added a comment.EditedFeb 15 2015, 6:03 PM

Thinking about this a little more, I'm conflicted as to whether or not it should allow the simplification if the boolean constant came from a macro. If the macro is a feature detection macro, then I'm enabling/disabling chunks of code at runtime based on the resolution of the feature detection and I shouldn't be so quick to simplify that to always true or always false.

I think this is similar to the constexpr case where if the desired result is to simplify the application of the macro, then the macro should be inlined first.

Guard against false positives from macro expansion resulting in a boolean literal

In article <CAPtQZ7-7UQAN9KcoQ8XGKht16tiFaRBHq0yM7=qTz-ucek0MkQ@mail.gmail.com>,

Ismail Pazarbasi <ismail.pazarbasi@gmail.com> writes:

Yes, that was my concern. I think we should avoid correcting or
warning about macros eagerly.

I agree.

// foo.h
#define EXPECT_FALSE(x) ((x) == false) // macro written in a header file

// foo.cpp
#include "foo.h"
#define MY_EXPECT_FALSE(x) ((x) == false)
static void f() {
  if (EXPECT_FALSE(0 == 1)); // ok
  if (MY_EXPECT_FALSE(0 == 1)); // warn
}

I will add this (or something similar) to my FileCheck test suite.

I was using isExpansionInMainFile() to avoid matching any system
included headers, but perhaps I was misunderstanding how that
narrowing matcher works.

alexfh edited edge metadata.Feb 25 2015, 7:17 AM

This is going to be a really nice check if it's done right. Thanks for the contribution!

There's another pattern (similar to the examples with ternary expressions in the patch description) that annoys me:

if (<something>)
  return true;
else
  return false;

Could you add a support for it?

And a few style issues.

clang-tidy/readability/SimplifyBooleanExpr.cpp
16

I'd remove this and start namespace clang right after the next line.

34

I'd go with

const char Something[] = "...";

as it's more readable due to fewer consts.

282

Please use the style of Clang warnings: first letter is lower-case, no trailing period.

clang-tidy/readability/SimplifyBooleanExpr.h
26

Please clang-format the code with the LLVM style.

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

Please leave each distinct error message completely only once, and shorten all other copies of it.

62

Please clang-format the test with LLVM style and infinite column limit (to avoid breaking the CHECK lines).

sbenza added inline comments.Feb 25 2015, 10:42 AM
clang-tidy/readability/SimplifyBooleanExpr.cpp
21

Don't force a conversion to std::string. Keep this as StringRef to avoid the cost of the string.

51

There is no need to mark this inline.
Are you doing it to avoid ODR-problems? or to make the code "faster"?

53

Use StringRef instead of 'const char*'.
Here and all other 'const char*' arguments.

77

Why does it matter if the RHS has a bool literal in it?
This would prevent something like this from matching:

if (true && SomeFunction("arg", true))

Also, hasDescedant() and alike are somewhat expensive. Something to take into account when using them.

97

How are these different from matchBoolBinOpExpr ?

210

StringRef

221

All these replace look the same.
The are replacing the full expression with a subexpression.
They seem like they all could be:

void SimplifyBooleanExpr::replaceWithSubexpression(
        const ast_matchers::MatchFinder::MatchResult &Result,
        const Expr *Subexpr, StringRef Prefix = "")
{
    const Expr* Expression = Result.Nodes.getNodeAs<Expr>(ExpressionId);
    diag(Subexpr->getLocStart(), SimplifyDiagnostic)
        << FixItHint::CreateReplacement(Expression->getSourceRange(),
                                        (Prefix + getText(Result, *Subexpr)).str());
}

If all the replace calls look the same, then you can also consolidate the bind ids to simplify check().
It could be like:

if (const Expr* Sub = result.Nodes.getNodeAs<Expr>("ReplaceWithSub")) {
  replaceWithSubexpression(Result, Sub);
} else if (...

and that might actually support all the uses cases, in which case no if/else is needed. Then check() becomes:

void SimplifyBooleanExpr::check(const ast_matchers::MatchFinder::MatchResult &Result)
{
  const Expr* Full = Result.Nodes.getNodeAs<Expr>(ExpressionId);
  const Expr* Sub = Result.Nodes.getNodeAs<Expr>(SubexpressionId);
  StringRef Prefix = Result.Nodes.getNodeAs<Expr>(AddNotId) ? "!" : "";
  diag(Sub->getLocStart(), SimplifyDiagnostic)
      << FixItHint::CreateReplacement(Full->getSourceRange(),
                                      (Prefix + getText(Result, *Sub)).str());
}
clang-tidy/readability/SimplifyBooleanExpr.h
19

Comment is a copy-paste from some other check.

clang-tidy/readability/SimplifyBooleanExpr.cpp
51

I suppose it's my own coding habit to mark functions containing one or two statements as inline. There is nothing intrinsically important about this function being inline.

53

Yes, this came up in the review of readability-remove-void-arg and that change will be folded in here as well. Thanks.

77

There are two cases: either the boolean literal is the LHS or it is the RHS of a binary operator. This is the matcher that handles the LHS being the literal and the RHS being the expression.

This code specifically excludes the case where both LHS and RHS are boolean literals so that complex expressions of constants like:

if (false && (true || false))

are handled without error -- see the nested_booleans code in the test file. If I don't narrow the matcher in this way, then the above expression gets two "fixes" written for it, one that replaces the inner expression and another that replaces the outer expression. Unfortunately, when this happens the combined edits produce invalid code.

I am not certain if this is an error in the engine that applies the fixes or not, but that's what you currently get. So to prevent invalid code from being generated, I disallow one of the matches.

This check could be enhanced to do more elaborate boolean expression elimination, but realistically this is just a corner case for this checker and not something that you should expect to see in real-world code.

I'll look at seeing if there is an alternative to hasDescendant() that I can use that will do the same job.

97

I tried to name the functions after what they were matching. One is matching <bool> <binop> <expr> and the other is matching <expr> <binop> <bool>. (Sorry for the crappy notation there.)

If you didn't see the difference immediately with the names of the two functions, then I need better names! I'm open to suggestions :)

221

I'll look into seeing if I can simplify it further, but the differences come down to either the fact that they have different matchers or that the replacement text is different in the different cases.

Honestly, I abhor duplication and I've pounded on this for many hours trying to make it as small as I can and that's what you're seeing in these reviews.

clang-tidy/readability/SimplifyBooleanExpr.h
19

D'oh! Thanks for catching this and your other comments as well, they've been very helpful.

clang-tidy/readability/SimplifyBooleanExpr.cpp
16

Is it acceptable for me to add anonymous namespaces inside namespace clang?

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

Many of the tests are specifically designed to show that the user's whitespace is unmolested.

If I reformat the test file using clang-format, it will break all these tests.

A user can always run clang-format on the output of clang-tidy if they want LLVM formatting on the result.

LegalizeAdulthood updated this object.
LegalizeAdulthood edited edge metadata.

Corrected documentation comments.
Support simplification of if (e) return true; else return false;
Fixed formatting of diagnostic messages to be consistent with the rest of clang.
Eliminated duplication of warning messages in tests.
clang-format -style=LLVM applied to the new files
Prefer StringRefs over std::string.
Prefer auto when initializing variables.
Let the compiler decide what to inline.

Drop some unnecessary namespace qualifications.

alexfh added inline comments.Mar 2 2015, 6:46 AM
clang-tidy/readability/SimplifyBooleanExpr.cpp
237

Any luck replacing replaceWith* methods with a single one as Samuel suggested?

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

I agree that the tests that verify whitespace handling can have any formatting that suites their needs.

But do you need, for example, the lines in this test be indented by 4 spaces and opening braces be placed on the next line after function header? I think, we should follow the LLVM style in tests (except for the few places where other formatting is required for the purposes of the test).

alexfh added inline comments.Mar 2 2015, 6:46 AM
clang-tidy/readability/SimplifyBooleanExpr.cpp
21

Is this function used only once? Maybe inline it then?

33

This form is rather exotic in Clang and LLVM code. I'd still prefer const char X[] = "...";.

54

Here and in a few other places in this patch: please use const X * instead of X const * (and the same for references) for consistency with other clang-tidy code.

55

Please use StringRef instead of const char* wherever it makes sense.

58

nit: It looks better with parentheses and makes operator precedence clear:

return (Literal &&
        Result.SourceManager->isMacroBodyExpansion(Literal->getLocStart()))
           ? nullptr
           : Literal;
67

How about putting this into the ReturnsBool function?

auto SimpleReturnsBool =
    returnStmt(has(boolLiteral(equals(Value)).bind(Id ? Id : "ignored")));
301

We use auto when it's clear what's behind it or the alternative isn't better. The most frequent use of auto is when the type (or its important part) is spelled in the RHS of the initialization. It also frequently makes sense to use auto in range-based for loops.

Here I would clearly use StringRef instead.

The comment applies to other usages of auto in this file as well.

302
isa<CompoundStmt>(If->getElse()) ? ";" : "";
305

And here the use of auto is simply dangerous: this creates a Twine object that is only supposed to be used as a temporary (because if your getText() returned std::string, the Twine object would point to a deleted memory region right after the end of the expression). Please use std::string instead.

307

Also not the best use for auto. Please use SourceLocation instead.

clang-tidy/readability/SimplifyBooleanExpr.h
24–35

Some beutification wouldn't harm, e.g.:

///   `if (b == true)`                           becomes `if (b)`
///   `if (b == false)`                          becomes `if (!b)`
///   `if (b && true)`                           becomes `if (b)`
///   `if (b && false)`                          becomes `if (false)`
///   `if (b || true)`                           becomes `if (true)`
///   `if (b || false)`                          becomes `if (b)`
///   `e ? true : false`                         becomes `e`
///   `e ? false : true`                         becomes `!e`
///   `if (true) t(); else f();`                 becomes `t();`
///   `if (false) t(); else f();`                becomes `f();`
///   `if (e) return true; else return false;`   becomes `return (e);`
///   `if (e) return false; else return true;`   becomes `return !(e);`
sbenza added inline comments.Mar 2 2015, 8:17 AM
clang-tidy/readability/SimplifyBooleanExpr.cpp
77

There are a few ways to do this.
A "simple" one is to not restrict the matchers and keep a list of fixed SourceRanges in the check.
Then check the list to see if you are about to fix a subrange of something you already fixed.
(if you do this, you must reset this local state at the end of the translation unit).
The good this about this approach is that the matchers are simple and it is easier to not have false negatives.

97

I noticed the difference in the name, but the body is pretty much the same thing.
If you think the name is good documentation, then it is fine to keep them separate.

221

From what I can see, they (most?) all:

  • Replace the full expression with a subexpression.
  • Optionally add a prefix (a '!') to the replacement.

For all the cases that do this, you can handle them with the same check() logic. Each case would need to:

  • bind() the full expression that needs to be replaced.
  • bind() the subexpression that will be the replacement.
  • optionally mention whether it needs the "!" prefix. You can do this by binding any arbitrary node.

Then you can have code like:

void SimplifyBooleanExpr::matchBoolBinOpExpr(MatchFinder *Finder, bool Value,
                                             const char *const OperatorName) {
  Finder->addMatcher(
      binaryOperator(expr().bind("Full"),
                     isExpansionInMainFile(), hasOperatorName(OperatorName),
                     hasLHS(boolLiteral(equals(Value)),
                     hasRHS(expr().bind("Replacement")),
                     unless(hasRHS(hasDescendant(boolLiteral())))),
      this);

void SimplifyBooleanExpr::matchExprCompOpBool(MatchFinder *Finder, bool Value,
                                              const char *const OperatorName) {
  Finder->addMatcher(
      binaryOperator(
          expr().bind("Full"),
          expr().bind("Negate"),
          isExpansionInMainFile(), hasOperatorName(OperatorName),
          hasLHS(expr().bind(ExpressionId)).bind("Replacement"),
          unless(hasLHS(boolLiteral())),
          unless(hasLHS(hasDescendant(boolLiteral()))),
          hasRHS(hasDescendant(boolLiteral(equals(Value))))),
      this);
}


void SimplifyBooleanExpr::check(const ast_matchers::MatchFinder::MatchResult &Result)
{
  // Handle all the simple cases (are they all like this?)
  if (const Expr* Replacement = Result.Nodes.getNodeAs<Expr>("Replacement")) {
    const Expr* Full = Result.Nodes.getNodeAs<Expr>("Full");
    StringRef Prefix = Result.Nodes.getNodeAs<Expr>("Negate") ? "!" : "";
    diag(Replacement->getLocStart(), SimplifyDiagnostic)
        << FixItHint::CreateReplacement(Full->getSourceRange(),
                                        (Prefix + getText(Result, *Replacement)).str());
  } else {
    // Handle other cases that do not have simple replacements.
  }
}
clang-tidy/readability/SimplifyBooleanExpr.cpp
21

I'm getting conflicting reviews here.

One reviewer wants me not to write inline on small functions and you're asking me to write inline.

I honestly don't care which, but please tell me which request I should follow :-).

54

Darnit, I thought I got all those :-). My fingers put the const on the right (how else do you write const pointer to const character?).

55

I'm happy to do that with some guidelines on what "makes sense". In this case it is character literals coming down from the place where you were saying you preferred const char Id[] = ....

What am I gaining by using StringRef here?

58

Talk to clang-format. I'm not using any of my own whitespace preferences, just ramming it through clang-format and taking what it gives me.

67

I went through a couple iterations on this where I had some auto lambdas and then went to named functions with explicit return types. For this one I don't think the auto is less readable.

237

I haven't done the experimenting yet, but I'm not hopeful. They really are all just different enough that I haven't yet seen a way to unify them.

302

Nice! I will be using that.

305

My head is spinning with all this farbling around between std::string, StringRef and Twine. Is the proper usage of these string classes documented somewhere so that I can refer to it while preparing a patch and spend less time churning the patch just to make these adjustments?

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

It's a lot of manual rework for no functional benefit.

What are we gaining?

clang-tidy/readability/SimplifyBooleanExpr.cpp
97

While they may be "pretty much the same", the differences are important because the replacements are very different due to short-circuiting with && and ||.

If there were no short-circuiting then it would be the simpler thing you're suggesting.

221

I'll take a look at this and see what I can do.

alexfh added inline comments.Mar 2 2015, 11:45 AM
clang-tidy/readability/SimplifyBooleanExpr.cpp
21

We're talking about different ways to inline a function. I mean inlining on the source code level: take the function body and use it in the single place that invokes it.

Sorry for the ambiguity ;)

55

"Makes sense" here means roughly "everywhere where you pass a string, and not a raw pointer to const char".

What am I gaining by using StringRef here?

Peace of mind (in some sense): when you see a StringRef argument, you know that there is no expensive copying or calculation of the length going on (except where you create a StringRef from a char pointer) . Just use StringRef everywhere where you need a constant string (or a fragment of a string), and there's a buffer with sufficient lifetime (string literal, std::string) somewhere.

You also get fewer code review comments to deal with ;)

58

clang-format doesn't add or remove parentheses. But if you add parentheses here yourself, you'll get the formatting as in the comment above.

So I ask you to add parentheses and re-run clang-format on this line.

Sorry again for not being clear.

67

Note, there's no lambda needed, and no function as well. You just need a certain matcher to use in a more complex matcher. auto SomeMatcher = someMatcher(...); is just the simplest way to write it. The function would make sense if you called it with different arguments, e.g.:

return anyOf(
    SimpleReturnsBool(Value, SomeId),
    compoundStmt(statementCountIs(1), has(SimpleReturnsBool(Value, SomeOtherId))));

Otherwise, it's just simpler and more readable without a separate function:

auto SimpleReturnsBool =                                                          
    returnStmt(has(boolLiteral(equals(Value)).bind(Id ? Id : "ignored")));        
return anyOf(SimpleReturnsBool,                                                   
             compoundStmt(statementCountIs(1), has(SimpleReturnsBool)));
305

Some documentation is here:
http://llvm.org/docs/ProgrammersManual.html#passing-strings-the-stringref-and-twine-classes

You can also read class comments on StringRef and Twine.

What's important here, is that any StringRef + something that compiles yields a Twine. And one dangerous case is StringRef + temporary std::string. This creates a Twine pointing to a temporary std::string. The temporary string gets deleted right at the end of the expression, and the Twine continues to point to the deleted memory. So one should (almost) never store a Twine. The correct way to use it is:

std::string Result = (llvm::Twine("...") + "..." + someFunctionReturningAStdStringOrAStringRef() + ...).str();

Fuss around with some types to get closer to clang style.
Simplify replacement of matching expressions by eliminating duplication.

alexfh added inline comments.Mar 3 2015, 7:04 AM
clang-tidy/readability/SimplifyBooleanExpr.cpp
21

What about this?

55

ditto

58

ditto

307

ditto

sbenza added inline comments.Mar 3 2015, 10:39 AM
clang-tidy/readability/SimplifyBooleanExpr.cpp
63

This type is an implementation detail.
Why not simply internal::Matcher<Stmt> ?

237

I'm sorry, but I have not seen one example that is really different.
They all do the same:

diag(<SomeLocationInTheExpression>, SimplifyOperatorDiagnostic)
    << FixItHint::CreateReplacement(
           <TheRangeOfTheFullExpression>,
           <OptionalPrefix> + getText(Result, <TheSubExpressionWeSelectedAsReplacement>));

I posted code on how to do this generically for all (most?) the cases. It does not hard code binary operations or anything else. Only 3 things are needed here and the matcher can bind to generic ids.

LegalizeAdulthood added a comment.EditedMar 3 2015, 2:07 PM

Thanks for the comments. I apologize for having to make the same comment repeatedly; I find it difficult to match up all the open issues in phabricator. I'm used to review board where such things are easier to manage. I try to work through the open issues, but since they are mixed in with earlier diffs and there's no way for me to mark an issue as fixed in phabricator, it makes me manually go through everything and make sure I got them and I end up missing some items. Please bear with me and have patience :).

As I've been working through the comments, I thought of another simplification that this check should do:

if (e) x = true; else x = false;
if (e) { x = true; } else { x = false; }

These should be simplified to x = e; and their negated counterparts.

clang-tidy/readability/SimplifyBooleanExpr.cpp
21

Fixed.

21

OK, so the first getText overload is on a source range.

The next getText is an overload on the template argument, so you get one instantiation per template argument type. They all call the first overload after extracting the source range for the node type.

Effectively, the first overload is the non-template parameter varying part of the algorithm and the second overload is the template parameter varying part.

33

Fixed

34

Fixed.

51

Fixed

53

Fixed

54

Fixed.

55

Fixed

58

Fixed

63

Fixed.

67

Fixed

77

I'm investigating alternatives to the current implementation.

210

Fixed

221

Fixed

237

Fixed

282

Fixed

301

Fixed

302

Fixed

305

Fixed

307

Fixed

clang-tidy/readability/SimplifyBooleanExpr.h
19

Fixed

24–35

Fixed.

26

Fixed.

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

Fixed

LegalizeAdulthood updated this object.

Add conditional assignment simplification.
Review all oustanding comments and address the issues raised.

alexfh added inline comments.Mar 10 2015, 7:49 AM
clang-tidy/readability/SimplifyBooleanExprCheck.cpp
254 ↗(On Diff #21425)

Please use "const auto *" for pointers.

315 ↗(On Diff #21425)
  1. Parentheses are not always needed. I'd suggest adding a couple of helper functions to format negation correctly. These may be missing something, but should be mostly correct:
  bool needsParensAfterUnaryNegation(const Expr &E) {
    if (isa<BinaryOperator>(E) || isa<ConditionalOperator>(E))
      return true;
    if (const auto &Op = dyn_cast<CXXOperatorCallExpr>(E))
      return Op->getNumArgs() == 2 && Op->getOperator() != OO_Call && Op->getOperator() != OO_Subscript;
    return false;
  }

std::string negateExpression(const MatchFinder::MatchResult &Result, const Expr &E) {
  StringRef Text = getText(Result, E);
  return (needsParensAfterUnaryNegation(E) ? "!(" + Text + ")" : "!" + Text).str();
}

This applies to other replaceWith* methods as well.

  1. How about moving out all non-conditional parts to shorten the expression?

    (VariableName + " = " + (Negated ? "!(" + Condition + ")" : Condition) + Terminator).str();
test/clang-tidy/readability-simplify-bool-expr.cpp
52

Why?

63

Please make the test code closer to LLVM style, except for the places where special formatting is required for testing whitespace handling. In particular: 2 space indentation, braces on the same line as function headers or control flow statements, function names should be camelCaseStartingWithALowerCaseLetter, comments should use proper capitalization and punctuation.

171

Why?

clang-tidy/readability/SimplifyBooleanExprCheck.cpp
254 ↗(On Diff #21425)

Fixed

315 ↗(On Diff #21425)

Fixed

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

Fixed

63

Fixed

171

Fixed

Extract Function replacementExpression
Reformat test file.

alexfh accepted this revision.Mar 19 2015, 6:53 PM
alexfh edited edge metadata.

Another couple of nits. Otherwise looks good!

Please fix the issues and have someone submit this for you. I won't be able to submit the patch in the next few days.

clang-tidy/readability/SimplifyBooleanExprCheck.cpp
244 ↗(On Diff #22241)

I had to look up what auto is here. Let's use const CXXBoolLiteralExpr *.

250 ↗(On Diff #22241)

const auto *, please.

This revision is now accepted and ready to land.Mar 19 2015, 6:53 PM
LegalizeAdulthood edited edge metadata.

Prefer explicit declarations over auto when initializing expression doesn't mention the type explicitly.
Prefer const auto * declaration when declaring a node type from getNodeAs<T>.
Unwrap parenthesis around replacement expression where possible.

clang-tidy/readability/SimplifyBooleanExprCheck.cpp
244 ↗(On Diff #22241)

Fixed

250 ↗(On Diff #22241)

Fixed

I'm finally back to work and got around to look at this patch again. The latest changes introduced a few issues (see inline comments). I can fix these and submit the patch for you.

clang-tidy/readability/SimplifyBooleanExprCheck.cpp
89 ↗(On Diff #22423)

nit: In LLVM style there should be no braces around a single-line if body.

101 ↗(On Diff #22423)

nit: Don't use else after a return.

105 ↗(On Diff #22423)

nit: This style seems to be more common in LLVM and more idiomatic: assert(false && "...").

116 ↗(On Diff #22423)

nit: Use const auto *.

118 ↗(On Diff #22423)

Text should be an std::string, otherwise you're storing a reference to a temporary string.

123 ↗(On Diff #22423)

nit: Use Text.empty() instead.

alexfh added inline comments.Apr 10 2015, 12:09 PM
clang-tidy/readability/SimplifyBooleanExprCheck.cpp
105 ↗(On Diff #22423)

This function can replace isNegatable, if it returns an empty StringRef instead of asserting. It will result in less code duplication.

alexfh closed this revision.Apr 10 2015, 12:30 PM

Fixed the issues and committed as r234626.

Thank you for contributing the check!

In article <76367d32bace035725762761e5da144f@localhost.localdomain>,

Alexander Kornienko <alexfh@google.com> writes:

I'm finally back to work and got around to look at this patch again. The
latest changes introduced a few issues (see inline comments). I can fix
these and submit the patch for you.

I'm finally able to revisit this, thanks for fixing those issues and
submitting it!

I have a few more polishing patches I'd like to apply to this to
make it more robust. One is in review right now (chained conditional
assignment/return). The other bit of polish I would like to apply is
handling when 'static_cast<bool>(e)' should be applied to the resulting
expression in order to preserve semantics in some edge cases.