[PATCH] Add readability-simplify-boolean-expr check to clang-tidy

[Alexander Kornienko]

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:20
@@ +19,3 @@
+
+StringRef getText(const MatchFinder::MatchResult &Result, SourceRange Range) {
+  return Lexer::getSourceText(CharSourceRange::getTokenRange(Range),
----------------
LegalizeAdulthood wrote:
> alexfh wrote:
> > Is this function used only once? Maybe inline it then?
> 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 :-).
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 ;)

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:54
@@ +53,3 @@
+CXXBoolLiteralExpr const *getBoolLiteral(MatchFinder::MatchResult const &Result,
+                                         const char *const Id) {
+  CXXBoolLiteralExpr const *Literal =
----------------
LegalizeAdulthood wrote:
> alexfh wrote:
> > Please use `StringRef` instead of `const char*` wherever it makes sense.
> 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?
"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 ;)

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:57
@@ +56,3 @@
+      Result.Nodes.getNodeAs<CXXBoolLiteralExpr>(Id);
+  return Literal &&
+                 Result.SourceManager->isMacroBodyExpansion(
----------------
LegalizeAdulthood wrote:
> alexfh wrote:
> > nit: It looks better with parentheses and makes operator precedence clear:
> > 
> >     return (Literal &&
> >             Result.SourceManager->isMacroBodyExpansion(Literal->getLocStart()))
> >                ? nullptr
> >                : Literal;
> > 
> 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.
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.

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:66
@@ +65,3 @@
+SimpleReturnsBool(bool Value, char const *const Id = nullptr) {
+  return returnStmt(has(boolLiteral(equals(Value)).bind(Id ? Id : "ignored")));
+}
----------------
LegalizeAdulthood wrote:
> alexfh wrote:
> > How about putting this into the `ReturnsBool` function?
> > 
> >   auto SimpleReturnsBool =
> >       returnStmt(has(boolLiteral(equals(Value)).bind(Id ? Id : "ignored")));
> 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.
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)));          

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:304
@@ +303,3 @@
+          : "";
+  const auto Replacement = "return " + StringRef(Negated ? "!" : "") + "(" +
+                           getText(Result, *If->getCond()) + ")" + Terminator;
----------------
LegalizeAdulthood wrote:
> alexfh wrote:
> > 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.
> 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?
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();

http://reviews.llvm.org/D7648

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/