[clang-tidy] readability-ternary-operator new check

[omtcyf0]

Summary

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.

[LegalizeAdulthood]

docs/clang-tidy/checks/readability-ternary-operator.rst

11

Why add the parentheses around the entire statement? They seem to serve no purpose.

What happens if the condition is an expression with the same or lower precedence than the ternary operator?

Most people seem to agree that (a || b) ? e1 : e2; is more readable than (a || b ? e1 : e2);

14

readability-simplify-boolean-expr does this when the return type is bool and the return statements are returning true and false.

It also handles conditional assignment via ternary operator when the value assigned is of type bool and the resulting expressions are boolean constants.

If this check also starts modifying the same ternary operators, then we can have two checks fighting to make changes if both checks are applied on the same run of clang-tidy. I'm unsure how best to resolve the conflict. I don't know what clang-tidy would do in such instances.

19

Are you also checking for intermediate preprocessor lines?

if (cond) {
#define BLAH 1
  return BLAH;
} else {
  return 0;
}

25

A slightly better wording:

If you want the check to perform an autofix, either reposition or remove the comments.

test/clang-tidy/readability-ternary-operator.cpp

4

You don't have any test cases where the condition is an expression with operators. Please add some tests for this, particularly operators of the same or lower precedence than the ternary operator.

[omtcyf0]

Followed some suggestions Richard has kindly given me; fixed the nonsense in SourceRanges' extracting code

[alexfh]

clang-tidy/readability/BracesAroundStatementsCheck.cpp

16

This file only needs one function from this namespace, so it's better to qualify the function name instead.

clang-tidy/readability/TernaryOperatorCheck.cpp

31

No need for braces around single-line if/for/... bodies. Here and elsewhere.

36

Leftover from debugging?

47

You could skip whole tokens here instead of one character at a time.

125

When you don't need the casted pointer, use isa<T> instead of dyn_cast<T>.

clang-tidy/readability/TernaryOperatorCheck.h

52

The function name is too vague. It basically says what type the function returns without saying what exactly it is.

docs/clang-tidy/checks/readability-ternary-operator.rst

10

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.

[omtcyf0]

Thanks for the review!

I'll fix these issues as soon as I manage to get time for that!

clang-tidy/readability/BracesAroundStatementsCheck.cpp

 //===--- BracesAroundStatementsCheck.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 "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) {
   assert(Loc.isValid());
   for (;;) {
     while (isWhitespace(*FullSourceLoc(Loc, SM).getCharacterData()))
       Loc = Loc.getLocWithOffset(1);
 

clang-tidy/readability/CMakeLists.txt

add_clang_library(clangTidyReadabilityModule
   InconsistentDeclarationParameterNameCheck.cpp
   NamedParameterCheck.cpp
   NamespaceCommentCheck.cpp
   ReadabilityTidyModule.cpp
   RedundantControlFlowCheck.cpp
   RedundantStringCStrCheck.cpp
   RedundantSmartptrGetCheck.cpp
   SimplifyBooleanExprCheck.cpp
+  TernaryOperatorCheck.cpp
   UniqueptrDeleteReleaseCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTooling
   )

clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
 #include "NamedParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
 #include "RedundantSmartptrGetCheck.h"
 #include "RedundantStringCStrCheck.h"
 #include "SimplifyBooleanExprCheck.h"
+#include "TernaryOperatorCheck.h"
 #include "UniqueptrDeleteReleaseCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 class ReadabilityModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<BracesAroundStatementsCheck>(
         "readability-braces-around-statements");
     CheckFactories.registerCheck<ContainerSizeEmptyCheck>(
         "readability-container-size-empty");
     CheckFactories.registerCheck<ElseAfterReturnCheck>(
         "readability-else-after-return");
     CheckFactories.registerCheck<FunctionSizeCheck>(
         "readability-function-size");
     CheckFactories.registerCheck<IdentifierNamingCheck>(
         "readability-identifier-naming");
     CheckFactories.registerCheck<ImplicitBoolCastCheck>(
         "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>(
         "readability-redundant-control-flow");
     CheckFactories.registerCheck<RedundantSmartptrGetCheck>(
         "readability-redundant-smartptr-get");
     CheckFactories.registerCheck<RedundantStringCStrCheck>(
         "readability-redundant-string-cstr");

clang-tidy/readability/TernaryOperatorCheck.h

+//===--- 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

clang-tidy/readability/TernaryOperatorCheck.cpp

+//===--- 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 hasTokensInside(const std::vector<tok::TokenKind> TokenKinds,
+                     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);
+    for (auto Kind : TokenKinds) {
+      if (Kind == TokKind) {
+        return true;
+      }
+    }
+    // Fast-forward current token.
+    // it = Lexer::getLocForEndOfToken(it, 0, SM, Context->getLangOpts());
+  }
+  return false;
+}
+
+SourceLocation getNextTokenLoc(tok::TokenKind TokKind, SourceLocation Loc,
+                               const SourceManager &SM,
+                               const ASTContext *Context, bool After = true) {
+  SourceLocation Result =
+      Lexer::getLocForEndOfToken(Loc, 0, SM, Context->getLangOpts());
+  while (lexer_utils::getTokenKind(Result, SM, Context) != TokKind) {
+    Result = Result.getLocWithOffset(1);
+  }
+  return Result;
+}
+
+SourceLocation getPrevTokenLoc(tok::TokenKind TokKind, SourceLocation Loc,
+                               const SourceManager &SM,
+                               const ASTContext *Context, bool After = true) {
+  SourceLocation Result =
+      Lexer::GetBeginningOfToken(Loc, SM, Context->getLangOpts());
+  while (lexer_utils::getTokenKind(Result, SM, Context) != TokKind) {
+    Result = Result.getLocWithOffset(-1);
+  }
+  return Result;
+}
+
+} // 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) &&
+      !hasTokensInside({tok::comment, tok::hash}, IfNode->getSourceRange(), *SM,
+                       Ctx)) {
+    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(),
+      getPrevTokenLoc(tok::l_paren, IfNode->getCond()->getLocStart(), *SM, Ctx)
+          .getLocWithOffset(-1));
+  Hints.push_back(FixItHint::CreateRemoval(NextRange));
+
+  NextRange = SourceRange(
+      getNextTokenLoc(tok::r_paren, IfNode->getCond()->getLocEnd(), *SM, Ctx),
+      ThenNode->getLocStart().getLocWithOffset(-1));
+  NextReplacement = ") ?";
+
+  Hints.push_back(FixItHint::CreateReplacement(NextRange, NextReplacement));
+  NextRange =
+      SourceRange(getNextTokenLoc(tok::semi, ThenNode->getLocEnd(), *SM, Ctx),
+                  ElseNode->getLocStart().getLocWithOffset(-1));
+  NextReplacement = " :";
+  Hints.push_back(FixItHint::CreateReplacement(NextRange, NextReplacement));
+
+  NextRange = SourceRange(
+      getNextTokenLoc(tok::semi, ElseNode->getLocEnd(), *SM, Ctx)
+          .getLocWithOffset(1),
+      (dyn_cast<CompoundStmt>(IfNode->getElse())
+           ? IfNode->getElse()->getLocEnd()
+           : getNextTokenLoc(tok::semi, ElseNode->getLocEnd(), *SM, Ctx)
+                 .getLocWithOffset(1)));
+  Hints.push_back(FixItHint::CreateRemoval(NextRange));
+
+  return Hints;
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tidy/utils/LexerUtils.h

 namespace tidy {
 namespace lexer_utils {
 
 // Returns previous non-comment token skipping over any comment text or
 // tok::unknown if not found.
 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
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_LEXER_UTILS_H

clang-tidy/utils/LexerUtils.cpp

if (!Lexer::getRawToken(Location, Token, SourceManager,
         !Token.is(tok::comment)) {
       break;
     }
     Location = Location.getLocWithOffset(-1);
   }
   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

docs/clang-tidy/checks/list.rst

.. toctree::
    readability-identifier-naming
    readability-implicit-bool-cast
    readability-inconsistent-declaration-parameter-name
    readability-named-parameter
    readability-redundant-control-flow
    readability-redundant-smartptr-get
    readability-redundant-string-cstr
    readability-simplify-boolean-expr
+   readability-ternary-operator
    readability-uniqueptr-delete-release

docs/clang-tidy/checks/readability-ternary-operator.rst

+.. 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 and intermediate preprocessor
+lines 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, either reposition or
+remove the comments and intermediate preprocessor lines.

test/clang-tidy/readability-ternary-operator.cpp

+// 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
+
+  if (true || /* whatever */ false) {
+    call_me(0);
+  } else {
+    or_me(/* whoops; */ 0);
+  }
+  // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use ternary operator
+
+  if (Condition) {
+#define BLAH 1
+    Variable++;
+  } else {
+    Variable--;
+  }
+  // CHECK-MESSAGES: :[[@LINE-6]]: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 (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);
+
+  // p00rly f0rm4tt3d c0d3
+  if ( true || false ) {true;} else false;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use ternary operator
+  // CHECK-FIXES: ( true || false ) ?true : false;
+
+  if (true)true;else false;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use ternary operator
+  // CHECK-FIXES: (true) ?true : false;
+
+  if (true){true;}else false;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use ternary operator
+  // CHECK-FIXES: (true) ?true : false;
+
+  if (true)true;else{false;}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use ternary operator
+  // CHECK-FIXES: (true) ?true :false;
+}
+
+//===----------------------------------------------------------------------===//
+// 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++;
+  }
+}