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

[Daniel Marjamäki]

Checked in with 239909.


================
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
----------------
alexfh wrote:
> 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).
Fixed

================
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,
----------------
alexfh wrote:
> nit: "content ... is" or "contents ... are"
> Also, s/to complex/too complex/.
Fixed

================
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.
----------------
alexfh wrote:
> nit: It's more correct to use plural here: "parentheses starting at the current position."
I rewrote this
    // If current token is a parenthesis, skip it.
maybe it can be improved further

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

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

================
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",
----------------
alexfh wrote:
> Looks like you're calling `MI->getArgumentNum(Arg)` twice. Maybe store the result?
Fixed

http://reviews.llvm.org/D9496

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