omtcyf0 created this revision.
omtcyf0 added reviewers: alexfh, LegalizeAdulthood, Eugene.Zelenko, djasper.
omtcyf0 added a subscriber: cfe-commits.
This patch introduces a proof-of-concept check for clang-tidy aiming to suggest
using ternary operator in order to improve readability.
It's currently really straightforward and "clever" heuristics for auto fixing
more sophisticated cases would be added later.
http://reviews.llvm.org/D17244
Files:
clang-tidy/readability/BracesAroundStatementsCheck.cpp
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/ReadabilityTidyModule.cpp
clang-tidy/readability/TernaryOperatorCheck.cpp
clang-tidy/readability/TernaryOperatorCheck.h
clang-tidy/utils/LexerUtils.cpp
clang-tidy/utils/LexerUtils.h
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/readability-ternary-operator.rst
test/clang-tidy/readability-ternary-operator.cpp
Index: test/clang-tidy/readability-ternary-operator.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/readability-ternary-operator.cpp
@@ -0,0 +1,110 @@
+// RUN: %check_clang_tidy %s readability-ternary-operator %t
+
+bool Condition = true;
+int Variable = 0;
+
+void call_me(int value);
+void or_me(int value);
+
+//===----------------------------------------------------------------------===//
+// Section #0: warnings without autofixes
+//===----------------------------------------------------------------------===//
+
+int foo() {
+ // regular expressions
+ if (Condition) {
+ Variable++; // comment
+ } else {
+ Variable--;
+ }
+ // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use ternary operator
+
+ if (Condition) {
+ call_me(0);
+ } else {
+ /* something */or_me(1);
+ }
+ // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use ternary operator
+
+ // autofixing is not currently supported for ReturnStmt's
+ if (Condition) {
+ return 0;
+ } else {
+ return 1;
+ }
+ // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use ternary operator
+}
+
+//===----------------------------------------------------------------------===//
+// Section #1: warnings with autofixes
+//===----------------------------------------------------------------------===//
+
+void boo() {
+ // regular expressions
+ if (Condition) {
+ Variable++;
+ } else {
+ Variable--;
+ }
+ // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use ternary operator
+ // CHECK-FIXES: (Condition ? Variable++ : Variable--);
+
+ if (Condition)
+ Variable++;
+ else {
+ Variable--;
+ }
+ // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use ternary operator
+ // CHECK-FIXES: (Condition ? Variable++ : Variable--);
+
+ if (Condition) {
+ Variable++;
+ } else
+ Variable--;
+ // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use ternary operator
+ // CHECK-FIXES: (Condition ? Variable++ : Variable--);
+
+ if (Condition)
+ Variable++;
+ else
+ Variable--;
+ // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use ternary operator
+ // CHECK-FIXES: (Condition ? Variable++ : Variable--);
+
+ if (true || /* whatever */ false) {
+ call_me(0);
+ } else {
+ or_me(/* whoops; */ 0);
+ }
+ // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use ternary operator
+ // CHECK-FIXES: (true || /* whatever */ false ? call_me(0) : or_me(/* whoops; */ 0));
+
+ if (Condition) {
+ call_me(0);
+ } else {
+ or_me(0);
+ }
+ // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use ternary operator
+ // CHECK-FIXES: (Condition ? call_me(0) : or_me(0));
+}
+
+//===----------------------------------------------------------------------===//
+// Section #2: no warnings and autofixes
+//===----------------------------------------------------------------------===//
+
+void baz() {
+ if (Condition) {
+ Variable = 0;
+ } else if (!Condition) {
+ Variable = 1;
+ } else {
+ Variable = 2;
+ }
+
+ if (Condition) {
+ Variable++;
+ Variable--;
+ } else {
+ Variable++;
+ }
+}
Index: docs/clang-tidy/checks/readability-ternary-operator.rst
===================================================================
--- /dev/null
+++ docs/clang-tidy/checks/readability-ternary-operator.rst
@@ -0,0 +1,24 @@
+.. title:: clang-tidy - readability-ternary-operator
+
+readability-ternary-operator
+============================
+
+This check suggests using ternary operator in where it's possible to do so.
+A typical case where it might improve readability follows this pattern::
+ if (condition) expression0; else expression1;
+And it gets transformed into::
+ (condition ? expression0 : expression1);
+
+Another situation when ternary operator would be suggested::
+ if (condition) return literal0; else return literal1;
+
+Autofixes
+----------------------------
+
+The autofixes are only suggested when no comments would be lost.
+
+Consider the following piece of code::
+ if (condition) /* comment */ expression0; else /* and here */ expression1;
+While trying to modify this into an equal piece of code with ternary operator
+both comments will be lost, therefore there will be no autofix suggested for
+such case. If you want the check to perform an autofix just remove the comments.
Index: docs/clang-tidy/checks/list.rst
===================================================================
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -94,4 +94,5 @@
readability-redundant-smartptr-get
readability-redundant-string-cstr
readability-simplify-boolean-expr
+ readability-ternary-operator
readability-uniqueptr-delete-release
Index: clang-tidy/utils/LexerUtils.h
===================================================================
--- clang-tidy/utils/LexerUtils.h
+++ clang-tidy/utils/LexerUtils.h
@@ -22,6 +22,9 @@
Token getPreviousNonCommentToken(const ASTContext &Context,
SourceLocation Location);
+tok::TokenKind getTokenKind(SourceLocation Location, const SourceManager &SM,
+ const ASTContext *Context);
+
} // namespace lexer_utils
} // namespace tidy
} // namespace clang
Index: clang-tidy/utils/LexerUtils.cpp
===================================================================
--- clang-tidy/utils/LexerUtils.cpp
+++ clang-tidy/utils/LexerUtils.cpp
@@ -34,6 +34,17 @@
return Token;
}
+tok::TokenKind getTokenKind(SourceLocation Location, const SourceManager &SM,
+ const ASTContext *Context) {
+ Token Tok;
+ SourceLocation Beginning =
+ Lexer::GetBeginningOfToken(Location, SM, Context->getLangOpts());
+ const bool Invalid =
+ Lexer::getRawToken(Beginning, Tok, SM, Context->getLangOpts());
+
+ return (Invalid ? tok::NUM_TOKENS : Tok.getKind());
+}
+
} // namespace lexer_utils
} // namespace tidy
} // namespace clang
Index: clang-tidy/readability/TernaryOperatorCheck.h
===================================================================
--- /dev/null
+++ clang-tidy/readability/TernaryOperatorCheck.h
@@ -0,0 +1,64 @@
+//===--- TernaryOperatorCheck.h - clang-tidy---------------------*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_TERNARY_OPERATOR_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_TERNARY_OPERATOR_H
+
+#include "../ClangTidy.h"
+#include <string>
+#include <vector>
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Check that suggests using ternary operator where it's possible to.
+///
+/// Before:
+///
+/// \code
+/// if (condition) {
+/// expression0;
+/// } else {
+/// expression1;
+/// }
+/// \endcode
+///
+/// After:
+///
+/// \code
+/// (condition ? expression0 : expression1);
+/// \endcode
+///
+/// Note that if no autofix would be suggested if any comments may be lost.
+///
+/// For the user-facing documentation and more examples see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-ternary-operator.html
+class TernaryOperatorCheck : public ClangTidyCheck {
+public:
+ TernaryOperatorCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+ bool matchIf(const IfStmt *IfNode);
+ std::vector<FixItHint> getHints(const IfStmt *IfNode, const Expr *ThenNode,
+ const Expr *ElseNode);
+
+ const SourceManager *SM;
+ const LangOptions *Opts;
+ ASTContext *Ctx;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_TERNARY_OPERATOR_H
Index: clang-tidy/readability/TernaryOperatorCheck.cpp
===================================================================
--- /dev/null
+++ clang-tidy/readability/TernaryOperatorCheck.cpp
@@ -0,0 +1,190 @@
+//===--- TernaryOperatorCheck.cpp - clang-tidy-----------------------------===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "TernaryOperatorCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "../utils/LexerUtils.h"
+
+#include <vector>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+namespace {
+
+bool hasCommentsInside(const SourceRange SR, const SourceManager *SM,
+ const ASTContext *Context) {
+ for (SourceLocation it = SR.getBegin(); it != SR.getEnd();
+ it = it.getLocWithOffset(1)) {
+ tok::TokenKind TokKind = lexer_utils::getTokenKind(it, *SM, Context);
+ if (TokKind == tok::comment) {
+ return true;
+ }
+
+ // Fast-forward current token.
+ it = Lexer::getLocForEndOfToken(it, 0, *SM, Context->getLangOpts());
+ }
+ return false;
+}
+
+SourceLocation getPastSemi(SourceLocation Loc, const SourceManager *SM,
+ const ASTContext *Context) {
+ SourceLocation Result = Loc;
+ while (lexer_utils::getTokenKind(Result, *SM, Context) != tok::semi) {
+ Result = Result.getLocWithOffset(1);
+ }
+ return Result;
+}
+
+SourceLocation getPreciseTokenLocPastEnd(SourceLocation Loc,
+ const SourceManager &SM,
+ const LangOptions &LangOpts) {
+ unsigned TokLen =
+ Lexer::MeasureTokenLength(SM.getSpellingLoc(Loc), SM, LangOpts);
+ return Loc.getLocWithOffset(TokLen);
+}
+
+} // namespace
+
+void TernaryOperatorCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(ifStmt().bind("IfNode"), this);
+}
+
+bool TernaryOperatorCheck::matchIf(const IfStmt *IfNode) {
+ if (IfNode->getElse() == nullptr) {
+ return false;
+ }
+ if (const auto *ElseIfStmt = dyn_cast<IfStmt>(IfNode->getElse())) {
+ if (ElseIfStmt->getElse() != nullptr) {
+ return false;
+ }
+ }
+ for (const auto Parent : Ctx->getParents(*IfNode)) {
+ if (const IfStmt *ParentIf = Parent.get<IfStmt>()) {
+ if (ParentIf->getElse() == IfNode) {
+ return false;
+ }
+ }
+ }
+ if (const auto *ThenNode = dyn_cast<CompoundStmt>(IfNode->getThen())) {
+ if (ThenNode->size() > 1) {
+ return false;
+ }
+ }
+ if (const auto *ElseNode = dyn_cast<CompoundStmt>(IfNode->getElse())) {
+ if (ElseNode->size() > 1) {
+ return false;
+ }
+ }
+ return true;
+}
+
+void TernaryOperatorCheck::check(const MatchFinder::MatchResult &Result) {
+ this->Opts = &Result.Context->getLangOpts();
+ this->SM = Result.SourceManager;
+ this->Ctx = Result.Context;
+
+ const auto *IfNode = Result.Nodes.getNodeAs<IfStmt>("IfNode");
+ if (!matchIf(IfNode)) {
+ return;
+ }
+
+ const auto *ThenNode =
+ (dyn_cast<CompoundStmt>(IfNode->getThen())
+ ? dyn_cast<CompoundStmt>(IfNode->getThen())->body_front()
+ : IfNode->getThen());
+ const auto *ElseNode =
+ (dyn_cast<CompoundStmt>(IfNode->getElse())
+ ? dyn_cast<CompoundStmt>(IfNode->getElse())->body_front()
+ : IfNode->getElse());
+
+ if (ThenNode->getStmtClass() != ElseNode->getStmtClass()) {
+ return;
+ }
+ if (IfNode->getLocStart().isMacroID()) {
+ return;
+ }
+
+ std::vector<FixItHint> Hints;
+ if (dyn_cast<Expr>(ThenNode)) {
+ Hints =
+ getHints(IfNode, dyn_cast<Expr>(ThenNode), dyn_cast<Expr>(ElseNode));
+ }
+
+ auto Diagnostic = diag(IfNode->getLocStart(), "use ternary operator");
+ for (auto Hint : Hints) {
+ Diagnostic.AddFixItHint(Hint);
+ }
+}
+
+std::vector<FixItHint> TernaryOperatorCheck::getHints(const IfStmt *IfNode,
+ const Expr *ThenNode,
+ const Expr *ElseNode) {
+ // Having if (condition) { expression0; else { expression1; }
+ // ^ ^ ^ ^ ^ ^ ^ ^
+ // 1 2 3 4 5 6 7 8
+ //
+ // Transform into: (condition ? expression0 : expression1);
+ // 1. Replace 1-2 range with "("
+ // 2. Replace 3-4 range with " ? "
+ // 3. Replace 5-6 range with " : "
+ // 4. Insert ")" before 7
+ // 5. Remove 7-8 range
+
+ SourceRange NextRange;
+ std::string NextReplacement;
+ std::vector<FixItHint> Hints;
+
+ NextRange =
+ SourceRange(IfNode->getLocStart(),
+ IfNode->getCond()->getLocStart().getLocWithOffset(-1));
+ if (hasCommentsInside(NextRange, SM, Ctx)) {
+ return std::vector<FixItHint>();
+ }
+ Hints.push_back(FixItHint::CreateReplacement(NextRange, "("));
+
+ NextRange = SourceRange(
+ getPreciseTokenLocPastEnd(IfNode->getCond()->getLocEnd(), *SM, *Opts),
+ ThenNode->getLocStart().getLocWithOffset(-1));
+ NextReplacement = " ?";
+ if (hasCommentsInside(NextRange, SM, Ctx)) {
+ return std::vector<FixItHint>();
+ }
+ Hints.push_back(FixItHint::CreateReplacement(NextRange, NextReplacement));
+
+ NextRange = SourceRange(getPastSemi(ThenNode->getLocEnd(), SM, Ctx),
+ ElseNode->getLocStart().getLocWithOffset(-1));
+ NextReplacement = " :";
+ if (hasCommentsInside(NextRange, SM, Ctx)) {
+ return std::vector<FixItHint>();
+ }
+ Hints.push_back(FixItHint::CreateReplacement(NextRange, NextReplacement));
+
+ NextRange = SourceRange(
+ getPastSemi(ElseNode->getLocEnd(), SM, Ctx).getLocWithOffset(1),
+ (dyn_cast<CompoundStmt>(IfNode->getElse())
+ ? IfNode->getElse()->getLocEnd()
+ : getPastSemi(ElseNode->getLocEnd(), SM, Ctx).getLocWithOffset(1)));
+ if (hasCommentsInside(NextRange, SM, Ctx)) {
+ return std::vector<FixItHint>();
+ }
+ Hints.push_back(FixItHint::CreateRemoval(NextRange));
+
+ Hints.push_back(FixItHint::CreateInsertion(
+ getPastSemi(ElseNode->getLocEnd(), SM, Ctx), ")"));
+
+ return Hints;
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/readability/ReadabilityTidyModule.cpp
===================================================================
--- clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -22,6 +22,7 @@
#include "RedundantSmartptrGetCheck.h"
#include "RedundantStringCStrCheck.h"
#include "SimplifyBooleanExprCheck.h"
+#include "TernaryOperatorCheck.h"
#include "UniqueptrDeleteReleaseCheck.h"
namespace clang {
@@ -45,6 +46,8 @@
"readability-implicit-bool-cast");
CheckFactories.registerCheck<InconsistentDeclarationParameterNameCheck>(
"readability-inconsistent-declaration-parameter-name");
+ CheckFactories.registerCheck<TernaryOperatorCheck>(
+ "readability-ternary-operator");
CheckFactories.registerCheck<readability::NamedParameterCheck>(
"readability-named-parameter");
CheckFactories.registerCheck<RedundantControlFlowCheck>(
Index: clang-tidy/readability/CMakeLists.txt
===================================================================
--- clang-tidy/readability/CMakeLists.txt
+++ clang-tidy/readability/CMakeLists.txt
@@ -15,6 +15,7 @@
RedundantStringCStrCheck.cpp
RedundantSmartptrGetCheck.cpp
SimplifyBooleanExprCheck.cpp
+ TernaryOperatorCheck.cpp
UniqueptrDeleteReleaseCheck.cpp
LINK_LIBS
Index: clang-tidy/readability/BracesAroundStatementsCheck.cpp
===================================================================
--- clang-tidy/readability/BracesAroundStatementsCheck.cpp
+++ clang-tidy/readability/BracesAroundStatementsCheck.cpp
@@ -10,30 +10,16 @@
#include "BracesAroundStatementsCheck.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchers.h"
-#include "clang/Lex/Lexer.h"
+#include "../utils/LexerUtils.h"
using namespace clang::ast_matchers;
+using namespace clang::tidy::lexer_utils;
namespace clang {
namespace tidy {
namespace readability {
namespace {
-tok::TokenKind getTokenKind(SourceLocation Loc, const SourceManager &SM,
- const ASTContext *Context) {
- Token Tok;
- SourceLocation Beginning =
- Lexer::GetBeginningOfToken(Loc, SM, Context->getLangOpts());
- const bool Invalid =
- Lexer::getRawToken(Beginning, Tok, SM, Context->getLangOpts());
- assert(!Invalid && "Expected a valid token.");
-
- if (Invalid)
- return tok::NUM_TOKENS;
-
- return Tok.getKind();
-}
-
SourceLocation forwardSkipWhitespaceAndComments(SourceLocation Loc,
const SourceManager &SM,
const ASTContext *Context) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
