[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),
----------------
alexfh wrote:
> 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 ;)
What about this?

================
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:
> 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 ;)
ditto

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

================
Comment at: clang-tidy/readability/SimplifyBooleanExpr.cpp:306
@@ +305,3 @@
+                           getText(Result, *If->getCond()) + ")" + Terminator;
+  const auto Start =
+      Result.Nodes.getNodeAs<CXXBoolLiteralExpr>(ThenLiteralId)->getLocStart();
----------------
alexfh wrote:
> Also not the best use for auto. Please use `SourceLocation` instead.
ditto

http://reviews.llvm.org/D7648

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