[PATCH] clang-tidy: check for repeated side effects in macro

[Alexander Kornienko]

================
Comment at: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp:54
@@ +53,3 @@
+    int SkipParenCount = 0;
+    for (const auto &T : MI->tokens()) {
+      // Bail out if the contents of the macro is containing keywords that are
----------------
I find it hard to read this function due to many nested control statements. Let's split this loop in two separate checks: 

  if (std::find_if(MI->tokens().begin(), MI->tokens().end(), [] (const Token &T) { return T.isOneOf(...);}) != MI->tokens.end())
    return;

and

  if (CountArgumentExpansions(Arg /* or ResultArgToks */, MI->tokens()) < 2)
    continue;

And move most of it to a new `CountArgumentExpansions` function (if you have a better name, feel free to use it).

================
Comment at: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp:56
@@ +55,3 @@
+      // Bail out if the contents of the macro is containing keywords that are
+      // making the macro to complex.
+      if (T.isOneOf(tok::question, tok::kw_if, tok::kw_else, tok::kw_switch,
----------------
nit: "content ... is" or "contents ... are"
Also, s/to complex/too complex/.

================
Comment at: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp:62
@@ +61,3 @@
+        return;
+      // Skip the whole parenthesis that is starting at the current position. If
+      // none is starting, then do not skip anything.
----------------
nit: It's more correct to use plural here: "parentheses starting at the current position."

================
Comment at: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp:75
@@ +74,3 @@
+      // If not existent, skip it.
+      if (TII == NULL)
+        continue;
----------------
s/NULL/nullptr/

================
Comment at: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp:113
@@ +112,3 @@
+
+      if (AT.is(tok::plusplus) || AT.is(tok::minusminus)) {
+        Check.diag(ResultArgToks->getLocation(),
----------------
Please use `isOneOf`.

================
Comment at: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp:117
@@ +116,3 @@
+                   "repeated in macro expansion")
+            << (MI->getArgumentNum(Arg) + 1) << Arg->getName();
+        Check.diag(MI->getDefinitionLoc(), "macro %0 defined here",
----------------
Looks like you're calling `MI->getArgumentNum(Arg)` twice. Maybe store the result?

http://reviews.llvm.org/D9496

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/