[PATCH] D17244: [clang-tidy] readability-ternary-operator new check

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.

================
Comment at: clang-tidy/readability/BracesAroundStatementsCheck.cpp:16
@@ -15,2 +15,3 @@
 using namespace clang::ast_matchers;
+using namespace clang::tidy::lexer_utils;
 
----------------
This file only needs one function from this namespace, so it's better to qualify the function name instead.

================
Comment at: clang-tidy/readability/TernaryOperatorCheck.cpp:31
@@ +30,3 @@
+    for (auto Kind : TokenKinds) {
+      if (Kind == TokKind) {
+        return true;
----------------
No need for braces around single-line `if`/`for`/... bodies. Here and elsewhere.

================
Comment at: clang-tidy/readability/TernaryOperatorCheck.cpp:36
@@ +35,3 @@
+    // Fast-forward current token.
+    // it = Lexer::getLocForEndOfToken(it, 0, SM, Context->getLangOpts());
+  }
----------------
Leftover from debugging?

================
Comment at: clang-tidy/readability/TernaryOperatorCheck.cpp:47
@@ +46,3 @@
+  while (lexer_utils::getTokenKind(Result, SM, Context) != TokKind) {
+    Result = Result.getLocWithOffset(1);
+  }
----------------
You could skip whole tokens here instead of one character at a time.

================
Comment at: clang-tidy/readability/TernaryOperatorCheck.cpp:125
@@ +124,3 @@
+  std::vector<FixItHint> Hints;
+  if (dyn_cast<Expr>(ThenNode) &&
+      !hasTokensInside({tok::comment, tok::hash}, IfNode->getSourceRange(), *SM,
----------------
When you don't need the casted pointer, use `isa<T>` instead of `dyn_cast<T>`.

================
Comment at: clang-tidy/readability/TernaryOperatorCheck.h:52
@@ +51,3 @@
+  bool matchIf(const IfStmt *IfNode);
+  std::vector<FixItHint> getHints(const IfStmt *IfNode, const Expr *ThenNode,
+                                  const Expr *ElseNode);
----------------
The function name is too vague. It basically says what type the function returns without saying what exactly it is.

================
Comment at: docs/clang-tidy/checks/readability-ternary-operator.rst:10
@@ +9,3 @@
+And it gets transformed into::
+  (condition ? expression0 : expression1);
+
----------------
I don't think this particular replacement makes the code any better. A ternary operator in the void context seems rather unnatural.

What does make sense, is pulling some common part from both branches of the `if`, when there is a common part, e.g.:

  if (c) a = x; else a = y; // => a = c ? x : y;
  if (c) return x; else return y; // => return c ? x : y;
  if (c) f(x); else f(y); // => f(c ? x : y);
  if (c) {
    SourceRange Range(Foo.getLocStart(), Bar.getLocEnd());
    diag << FixItHint::CreateReplacement(Range, "qqq");
  } else {
    SourceRange Range(Foo.getLocStart(), Bar.getLocEnd());
    diag << FixItHint::CreateReplacement(Range, "eee");
  }
  // => 
  // SourceRange Range(Foo.getLocStart(), Bar.getLocEnd());
  // diag << FixItHint::CreateReplacement(Range, c ? "qqq" : "eee");

etc.


http://reviews.llvm.org/D17244