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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
