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

[Alexander Kornienko]

Thank you for the update!


================
Comment at: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp:56
@@ +55,3 @@
+    for (const auto &T : MI->tokens()) {
+      // Bail if contents of Macro is containing keywords that are making the
+      // macro to complex. 
----------------
s/Bail/Bail out/, otherwise it means something different. Also, "the contents" and s/Macro/the macro/ (there's no variable named Macro).

================
Comment at: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp:59
@@ +58,3 @@
+      if (T.isOneOf(tok::question, tok::kw_if, tok::kw_else, tok::kw_switch,
+          tok::kw_case, tok::kw_break, tok::kw_while, tok::kw_do, tok::kw_for,
+          tok::kw_continue, tok::kw_goto, tok::kw_return))
----------------
clang-format, please.

================
Comment at: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp:62
@@ +61,3 @@
+        return;
+      // Skip the whole paranthesis that is starting at the current position. If
+      // none is starting then do not skip anything.
----------------
s/paranthesis/parenthesis/

================
Comment at: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp:63
@@ +62,3 @@
+      // Skip the whole paranthesis that is starting at the current position. If
+      // none is starting then do not skip anything.
+      if (SkipParen) {
----------------
Missing comma before "then".

================
Comment at: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp:83
@@ +82,3 @@
+      }
+      // If another macro is found within the macro definition, skip the macro and the eventual arguments.
+      if (TII->hasMacroDefinition()) {
----------------
The line is too long. Please clang-format.

================
Comment at: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp:86
@@ +85,3 @@
+        MacroInfo* MI = PP.getMacroDefinition(TII).getMacroInfo();
+        if (MI != NULL && MI->isFunctionLike())
+          SkipParen = true;
----------------
s/NULL/nullptr/

================
Comment at: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp:114
@@ +113,3 @@
+        Check.diag(ResultArgToks->getLocation(), "side effects in the %ordinal0 macro "
+                                                 "argument '%1' is repeated in "
+                                                 "macro expansion")
----------------
Maybe "side-effects ... _are_ repeated"?

================
Comment at: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp:117
@@ +116,3 @@
+            << (MI->getArgumentNum(Arg) + 1) << Arg->getName();
+        Check.diag(MI->getDefinitionLoc(), "%0 macro defined here",
+                   DiagnosticIDs::Note)
----------------
"macro %0 defined here" would read better (given %0 is the name of the macro).

================
Comment at: test/clang-tidy/misc-repeated-side-effects-in-macro.c:7
@@ +6,3 @@
+  ret = badA(a++, b);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: side effects in the first macro argument 'x' is repeated in macro expansion [misc-macro-repeated-side-effects]
+  ret = badA(++a, b);
----------------
I thought, "%ordinal0" would generate "1st", not "first". Did you run tests after making changes?

================
Comment at: test/clang-tidy/misc-repeated-side-effects-in-macro.c:26
@@ +25,3 @@
+// False positive: Repeated side effects is intentional.
+// It is hard to know when it's done by intention so right now we warn
+#define UNROLL(A)    {A A}
----------------
nit: missing trailing period.

================
Comment at: test/clang-tidy/misc-repeated-side-effects-in-macro.c:33
@@ +32,3 @@
+
+// do not produce a false positive on a strchr() macro, currently the '?'
+// triggers the test to bail, because it cannot evaluate __builtin_constant_p(c)
----------------
Please use proper capitalization and punctuation in comments. Here and below.

================
Comment at: test/clang-tidy/misc-repeated-side-effects-in-macro.c:34
@@ +33,3 @@
+// do not produce a false positive on a strchr() macro, currently the '?'
+// triggers the test to bail, because it cannot evaluate __builtin_constant_p(c)
+#  define strchrs(s, c) \
----------------
Did you mean "bail out"? IMHO, "bail" means something different.

================
Comment at: test/clang-tidy/misc-repeated-side-effects-in-macro.c:44
@@ +43,3 @@
+void pass(char* pstr, char ch) {
+	strchrs(pstr, ch++); //no error
+}
----------------
Space after `//`, capitalization, trailing period.

================
Comment at: test/clang-tidy/misc-repeated-side-effects-in-macro.c:47
@@ +46,3 @@
+
+//check large argument (t=20, u=21)
+#define largeA(a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p,q,r,s,t,u,v,x,y,z) ((a)+(a)+(b)+(b)+(c)+(c)+(d)+(d)+(e)+(e)+(f)+(f)+(g)+(g)+(h)+(h)+(i)+(i)+(j)+(j)+(k)+(k)+(l)+(l)+(m)+(m)+(n)+(n)+(o)+(o)+(p)+(p)+(q)+(q)+(r)+(r)+(s)+(s)+(t)+(t)+(u)+(u)+(v)+(v)+(x)+(x)+(y)+(y)+(z)+(z))
----------------
ditto

================
Comment at: test/clang-tidy/misc-repeated-side-effects-in-macro.c:48
@@ +47,3 @@
+//check large argument (t=20, u=21)
+#define largeA(a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p,q,r,s,t,u,v,x,y,z) ((a)+(a)+(b)+(b)+(c)+(c)+(d)+(d)+(e)+(e)+(f)+(f)+(g)+(g)+(h)+(h)+(i)+(i)+(j)+(j)+(k)+(k)+(l)+(l)+(m)+(m)+(n)+(n)+(o)+(o)+(p)+(p)+(q)+(q)+(r)+(r)+(s)+(s)+(t)+(t)+(u)+(u)+(v)+(v)+(x)+(x)+(y)+(y)+(z)+(z))
+void large(int a) {
----------------
If the test doesn't need special formatting, please clang-format.

================
Comment at: test/clang-tidy/misc-repeated-side-effects-in-macro.c:72
@@ +71,3 @@
+
+// bailout for conditionals
+#define condA(x,y)  ((x)>(y)?(x):(y))
----------------
s/bailout/bail out/

http://reviews.llvm.org/D9496

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