[clang-tidy] Add check misc-multiple-statement-macro

Summary:
The check detects multi-statement macros that are used in unbraced conditionals.
Only the first statement will be part of the conditionals and the rest will fall
outside of it and executed unconditionally.

Reviewers: alexfh

Subscribers: cfe-commits

Differential Revision: http://reviews.llvm.org/D18766

llvm-svn: 266369

7 files changed, 249 insertions, 0 deletions

diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
index b5a5d53d4cf..c3ee5c3a6b2 100644
--- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
@@ -17,6 +17,7 @@ add_clang_library(clangTidyMiscModule
   MisplacedWideningCastCheck.cpp
   MoveConstantArgumentCheck.cpp
   MoveConstructorInitCheck.cpp
+  MultipleStatementMacroCheck.cpp
   NewDeleteOverloadsCheck.cpp
   NoexceptMoveConstructorCheck.cpp
   NonCopyableObjects.cpp
diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
index efbcde7245a..df335fa6e51 100644
--- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
@@ -25,6 +25,7 @@
 #include "MisplacedWideningCastCheck.h"
 #include "MoveConstantArgumentCheck.h"
 #include "MoveConstructorInitCheck.h"
+#include "MultipleStatementMacroCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
 #include "NonCopyableObjects.h"
@@ -79,6 +80,8 @@ public:
         "misc-move-const-arg");
     CheckFactories.registerCheck<MoveConstructorInitCheck>(
         "misc-move-constructor-init");
+    CheckFactories.registerCheck<MultipleStatementMacroCheck>(
+        "misc-multiple-statement-macro");
     CheckFactories.registerCheck<NewDeleteOverloadsCheck>(
         "misc-new-delete-overloads");
     CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
diff --git a/clang-tools-extra/clang-tidy/misc/MultipleStatementMacroCheck.cpp b/clang-tools-extra/clang-tidy/misc/MultipleStatementMacroCheck.cpp
new file mode 100644
index 00000000000..b7001aee802
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/misc/MultipleStatementMacroCheck.cpp
@@ -0,0 +1,106 @@
+//===--- MultipleStatementMacroCheck.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 "MultipleStatementMacroCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+namespace {
+
+AST_MATCHER(Expr, isInMacro) { return Node.getLocStart().isMacroID(); }
+
+/// \brief Find the next statement after `S`.
+const Stmt *nextStmt(const MatchFinder::MatchResult &Result, const Stmt *S) {
+  auto Parents = Result.Context->getParents(*S);
+  if (Parents.empty())
+    return nullptr;
+  const Stmt *Parent = Parents[0].get<Stmt>();
+  if (!Parent)
+    return nullptr;
+  const Stmt* Prev = nullptr;
+  for (const Stmt *Child : Parent->children()) {
+    if (Prev == S)
+      return Child;
+    Prev = Child;
+  }
+  return nextStmt(Result, Parent);
+}
+
+using ExpansionRanges = std::vector<std::pair<SourceLocation, SourceLocation>>;
+
+/// \bried Get all the macro expansion ranges related to `Loc`.
+///
+/// The result is ordered from most inner to most outer.
+ExpansionRanges getExpansionRanges(SourceLocation Loc,
+                                   const MatchFinder::MatchResult &Result) {
+  ExpansionRanges Locs;
+  while (Loc.isMacroID()) {
+    Locs.push_back(Result.SourceManager->getImmediateExpansionRange(Loc));
+    Loc = Locs.back().first;
+  }
+  return Locs;
+}
+
+}  // namespace
+
+void MultipleStatementMacroCheck::registerMatchers(MatchFinder *Finder) {
+  const auto Inner = expr(isInMacro(), unless(compoundStmt())).bind("inner");
+  Finder->addMatcher(
+      stmt(anyOf(ifStmt(hasThen(Inner)), ifStmt(hasElse(Inner)).bind("else"),
+                 whileStmt(hasBody(Inner)), forStmt(hasBody(Inner))))
+          .bind("outer"),
+      this);
+}
+
+void MultipleStatementMacroCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *Inner = Result.Nodes.getNodeAs<Expr>("inner");
+  const auto *Outer = Result.Nodes.getNodeAs<Stmt>("outer");
+  const auto *Next = nextStmt(Result, Outer);
+  if (!Next)
+    return;
+
+  SourceLocation OuterLoc = Outer->getLocStart();
+  if (Result.Nodes.getNodeAs<Stmt>("else"))
+    OuterLoc = cast<IfStmt>(Outer)->getElseLoc();
+
+  auto InnerRanges = getExpansionRanges(Inner->getLocStart(), Result);
+  auto OuterRanges = getExpansionRanges(OuterLoc, Result);
+  auto NextRanges = getExpansionRanges(Next->getLocStart(), Result);
+
+  // Remove all the common ranges, starting from the top (the last ones in the
+  // list).
+  while (!InnerRanges.empty() && !OuterRanges.empty() && !NextRanges.empty() &&
+         InnerRanges.back() == OuterRanges.back() &&
+         InnerRanges.back() == NextRanges.back()) {
+    InnerRanges.pop_back();
+    OuterRanges.pop_back();
+    NextRanges.pop_back();
+  }
+
+  // Inner and Next must have at least one more macro that Outer doesn't have,
+  // and that range must be common to both.
+  if (InnerRanges.empty() || NextRanges.empty() ||
+      InnerRanges.back() != NextRanges.back())
+    return;
+
+  diag(InnerRanges.back().first, "multiple statement macro used without "
+                                 "braces; some statements will be "
+                                 "unconditionally executed");
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
diff --git a/clang-tools-extra/clang-tidy/misc/MultipleStatementMacroCheck.h b/clang-tools-extra/clang-tidy/misc/MultipleStatementMacroCheck.h
new file mode 100644
index 00000000000..77a6b27dcde
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/misc/MultipleStatementMacroCheck.h
@@ -0,0 +1,37 @@
+//===--- MultipleStatementMacroCheck.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_MISC_MULTIPLE_STATEMENT_MACRO_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MULTIPLE_STATEMENT_MACRO_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Detect multiple statement macros that are used in unbraced conditionals.
+/// Only the first statement of the macro will be inside the conditional and the
+/// other ones will be executed unconditionally.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-multiple-statement-macro.html
+class MultipleStatementMacroCheck : public ClangTidyCheck {
+public:
+  MultipleStatementMacroCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MULTIPLE_STATEMENT_MACRO_H
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 99414bceaac..5fab245a14c 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -61,6 +61,7 @@ Clang-Tidy Checks
    misc-macro-repeated-side-effects
    misc-misplaced-widening-cast
    misc-move-constructor-init
+   misc-multiple-statement-macro
    misc-new-delete-overloads
    misc-noexcept-move-constructor
    misc-non-copyable-objects
diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc-multiple-statement-macro.rst b/clang-tools-extra/docs/clang-tidy/checks/misc-multiple-statement-macro.rst
new file mode 100644
index 00000000000..b2b47601929
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/misc-multiple-statement-macro.rst
@@ -0,0 +1,16 @@
+.. title:: clang-tidy - misc-multiple-statement-macro
+
+misc-multiple-statement-macro
+=============================
+
+Detect multiple statement macros that are used in unbraced conditionals.
+Only the first statement of the macro will be inside the conditional and the other ones will be executed unconditionally.
+
+Example:
+
+.. code:: c++
+
+  #define INCREMENT_TWO(x, y) (x)++; (y)++
+  if (do_increment)
+    INCREMENT_TWO(a, b);  // `(b)++;` will be executed unconditionally.
+
diff --git a/clang-tools-extra/test/clang-tidy/misc-multiple-statement-macro.cpp b/clang-tools-extra/test/clang-tidy/misc-multiple-statement-macro.cpp
new file mode 100644
index 00000000000..c943ee7c0f0
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/misc-multiple-statement-macro.cpp
@@ -0,0 +1,85 @@
+// RUN: %check_clang_tidy %s misc-multiple-statement-macro %t
+
+void F();
+
+#define BAD_MACRO(x) \
+  F();               \
+  F()
+
+#define GOOD_MACRO(x) \
+  do {                \
+    F();              \
+    F();              \
+  } while (0)
+
+#define GOOD_MACRO2(x) F()
+
+#define GOOD_MACRO3(x) F();
+
+#define MACRO_ARG_MACRO(X) \
+  if (54)                  \
+  X(2)
+
+#define ALL_IN_MACRO(X) \
+  if (43)               \
+    F();                \
+  F()
+
+#define GOOD_NESTED(x)   \
+  if (x)            \
+    GOOD_MACRO3(x); \
+  F();
+
+#define IF(x) if(x)
+
+void positives() {
+  if (1)
+    BAD_MACRO(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: multiple statement macro used without braces; some statements will be unconditionally executed [misc-multiple-statement-macro]
+  if (1) {
+  } else
+    BAD_MACRO(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: multiple statement macro used
+  while (1)
+    BAD_MACRO(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: multiple statement macro used
+  for (;;)
+    BAD_MACRO(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: multiple statement macro used
+
+  MACRO_ARG_MACRO(BAD_MACRO);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: multiple statement macro used
+  MACRO_ARG_MACRO(F(); int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: multiple statement macro used
+  IF(1) BAD_MACRO(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: multiple statement macro used
+}
+
+void negatives() {
+  if (1) {
+    BAD_MACRO(1);
+  } else {
+    BAD_MACRO(1);
+  }
+  while (1) {
+    BAD_MACRO(1);
+  }
+  for (;;) {
+    BAD_MACRO(1);
+  }
+
+  if (1)
+    GOOD_MACRO(1);
+  if (1) {
+    GOOD_MACRO(1);
+  }
+  if (1)
+    GOOD_MACRO2(1);
+  if (1)
+    GOOD_MACRO3(1);
+
+  MACRO_ARG_MACRO(GOOD_MACRO);
+  ALL_IN_MACRO(1);
+
+  IF(1) GOOD_MACRO(1);
+}