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

[sbenza]

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.

[etienneb]

this is cool.

clang-tidy/misc/MultipleStatementMacroCheck.cpp

33 ↗

(On Diff #52585)

I feel it nicer if you merge this namespace with the one at line 20.

42 ↗

(On Diff #52585)

Why not this?

if (found)
  return Child;
else
  found = Child == S;    // why the "|" is needed?

75 ↗

(On Diff #52585)

nit: no { }

[sbenza]

Minor fixes

[sbenza]

clang-tidy/misc/MultipleStatementMacroCheck.cpp

33 ↗

(On Diff #52585)

I like to put the helper functions closer to where they are used.
Merging the namespaces means I have to put everything at the top.

42 ↗

(On Diff #52585)

Fixed a different way.

[etienneb]

thanks, lgtm.

[hokein]

Nice check! I like the check, it would be very helpful. (I have struggled with this kind of bug before)

clang-tidy/misc/MultipleStatementMacroCheck.cpp

33 ↗

(On Diff #52625)

I think you can put this anonymous namespace into the above one.

37 ↗

(On Diff #52625)

Should we check the result returned by Result.Context->getParents is valid or not?

docs/clang-tidy/checks/misc-multiple-statement-macro.rst

15 ↗

(On Diff #52625)

Would be better to add a comment to explain the sample.

[sbenza]

Minor fixes

docs/clang-tidy/checks/misc-multiple-statement-macro.rst

15 ↗

(On Diff #52625)

The sentence just before the example explains what the problem is.

[alexfh]

LG with a nit.

clang-tidy/misc/MultipleStatementMacroCheck.cpp

99 ↗

(On Diff #52702)

If I saw this message from a tool, I wouldn't get what it means right away. Can you come up with an easier to read alternative? I can only suggest multiple statement macro used without braces, but maybe a more self-documenting message comes to your mind.

docs/clang-tidy/checks/misc-multiple-statement-macro.rst

16 ↗

(On Diff #52702)

While I see your point, this doesn't seem worse to me:

if (do_increment)
  INCREMENT_TWO(a, b); // `(b)++;` will always be executed.

Up to you though.

[sbenza]

Change warning message.

clang-tidy/misc/MultipleStatementMacroCheck.cpp

99 ↗

(On Diff #52702)

Sure.
I wrote my methodology to find the problem, not the actual problem.
What about this?

[alexfh]

clang-tidy/misc/MultipleStatementMacroCheck.cpp

100 ↗

(On Diff #53398)

Now it seems clear to me. Thanks!

clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt

add_clang_library(clangTidyMiscModule
   IncorrectRoundings.cpp
   InefficientAlgorithmCheck.cpp
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp
   MiscTidyModule.cpp
   MisplacedWideningCastCheck.cpp
   MoveConstantArgumentCheck.cpp
   MoveConstructorInitCheck.cpp
+  MultipleStatementMacroCheck.cpp
   NewDeleteOverloadsCheck.cpp
   NoexceptMoveConstructorCheck.cpp
   NonCopyableObjects.cpp
   SizeofContainerCheck.cpp
   StaticAssertCheck.cpp
   StringIntegerAssignmentCheck.cpp
   StringLiteralWithEmbeddedNulCheck.cpp
   SuspiciousMissingCommaCheck.cpp

clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp

 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundings.h"
 #include "InefficientAlgorithmCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MisplacedWideningCastCheck.h"
 #include "MoveConstantArgumentCheck.h"
 #include "MoveConstructorInitCheck.h"
+#include "MultipleStatementMacroCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
 #include "NonCopyableObjects.h"
 #include "SizeofContainerCheck.h"
 #include "StaticAssertCheck.h"
 #include "StringIntegerAssignmentCheck.h"
 #include "StringLiteralWithEmbeddedNulCheck.h"
 #include "SuspiciousMissingCommaCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<MacroRepeatedSideEffectsCheck>(
         "misc-macro-repeated-side-effects");
     CheckFactories.registerCheck<MisplacedWideningCastCheck>(
         "misc-misplaced-widening-cast");
     CheckFactories.registerCheck<MoveConstantArgumentCheck>(
         "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>(
         "misc-noexcept-move-constructor");
     CheckFactories.registerCheck<NonCopyableObjectsCheck>(
         "misc-non-copyable-objects");
     CheckFactories.registerCheck<SizeofContainerCheck>("misc-sizeof-container");
     CheckFactories.registerCheck<StaticAssertCheck>(

clang-tools-extra/trunk/clang-tidy/misc/MultipleStatementMacroCheck.h

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

clang-tools-extra/trunk/clang-tidy/misc/MultipleStatementMacroCheck.cpp

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

clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

.. toctree::
    misc-forward-declaration-namespace
    misc-inaccurate-erase
    misc-incorrect-roundings
    misc-inefficient-algorithm
    misc-macro-parentheses
    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
    misc-sizeof-container
    misc-static-assert
    misc-string-integer-assignment
    misc-string-literal-with-embedded-nul
    misc-suspicious-missing-comma

clang-tools-extra/trunk/docs/clang-tidy/checks/misc-multiple-statement-macro.rst

+.. 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.
+

clang-tools-extra/trunk/test/clang-tidy/misc-multiple-statement-macro.cpp

+// 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);
+}