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

[Richard]

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:20
@@ +19,3 @@
+
+StringRef getText(const MatchFinder::MatchResult &Result, SourceRange Range) {
+  return Lexer::getSourceText(CharSourceRange::getTokenRange(Range),
----------------
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 :-).

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:53
@@ +52,3 @@
+
+CXXBoolLiteralExpr const *getBoolLiteral(MatchFinder::MatchResult const &Result,
+                                         const char *const Id) {
----------------
alexfh wrote:
> 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.
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?).

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:54
@@ +53,3 @@
+CXXBoolLiteralExpr const *getBoolLiteral(MatchFinder::MatchResult const &Result,
+                                         const char *const Id) {
+  CXXBoolLiteralExpr const *Literal =
----------------
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?

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:57
@@ +56,3 @@
+      Result.Nodes.getNodeAs<CXXBoolLiteralExpr>(Id);
+  return Literal &&
+                 Result.SourceManager->isMacroBodyExpansion(
----------------
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.

================
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")));
+}
----------------
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.

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:236
@@ +235,3 @@
+
+void SimplifyBooleanExpr::replaceWithLeftBooleanLiteral(
+    const MatchFinder::MatchResult &Result,
----------------
alexfh wrote:
> Any luck replacing `replaceWith*` methods with a single one as Samuel suggested?
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.

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:301
@@ +300,3 @@
+  const auto Terminator =
+      (If->getElse()->getStmtClass() == Stmt::StmtClass::CompoundStmtClass)
+          ? ";"
----------------
alexfh wrote:
>   isa<CompoundStmt>(If->getElse()) ? ";" : "";
Nice!  I will be using that.

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

================
Comment at: test/clang-tidy/readability-simplify-bool-expr.cpp:62
@@ +61,3 @@
+
+void if_with_bool_literal_condition()
+{
----------------
alexfh wrote:
> LegalizeAdulthood wrote:
> > alexfh wrote:
> > > Please clang-format the test with LLVM style and infinite column limit (to avoid breaking the CHECK lines).
> > 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.
> 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).
It's a lot of manual rework for no functional benefit.

What are we gaining?

http://reviews.llvm.org/D7648

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