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

[Sebastian Edman]

================
Comment at: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp:74
@@ +73,3 @@
+      // macro to complex.
+      if (T.is(tok::question) || T.is(tok::kw_if) || T.is(tok::kw_else) ||
+          T.is(tok::kw_switch) || T.is(tok::kw_case) || T.is(tok::kw_break) ||
----------------
alexfh wrote:
> This would also benefit from `Token::isOneOf` that you can move from clang/lib/Format/FormatToken.h to clang::Token.
Fixed (the addition of Token::IsOneOf will be added in another patch).

================
Comment at: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp:96
@@ +95,3 @@
+      // skip next parenthesis.
+      if (TII->hasMacroDefinition() || TII->getBuiltinID() != 0) {
+        SkipParen = true;
----------------
alexfh wrote:
> You seem to assume that the macro is function-like, but you'd better check it (`PP->getMacroDefinition(TII)->getMacroInfo()->isFunctionLike()` + all required nullptr checks).
Actually the SkipParen bails if the next token is not a left parenthesis, but there may be complications if a parenthesis is following a macro without arguments.

I will implement your Idea in the next patch. It seems more roboust.

================
Comment at: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp:123
@@ +122,3 @@
+      if (AT.is(tok::plusplus) || AT.is(tok::minusminus)) {
+        Check.diag(ResultArgToks->getLocation(), "side effects in the %0macro "
+                                                 "argument '%1' is repeated in "
----------------
alexfh wrote:
> Let's use standard facilities provided by the diagnostics engine:
> 
>   Check.diag(..., "side effects in the %ordinal0 macro argument ....
> 
> If you want this to be more verbose, you can combine this with a `%select{}` modifier to add spellings for the first few ordinals.
So it did exist, have to dig deeper next time... I replaced the Tiers-functions with %ordinal0 and I think it is satisfying.

================
Comment at: test/clang-tidy/misc-repeated-side-effects-in-macro.c:66
@@ +65,3 @@
+	ret += builtinB(a++);
+	// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: side effects in the first macro argument 'x' is repeated in macro expansion [misc-macro-repeated-side-effects]
+	ret += macroA(a++);
----------------
alexfh wrote:
> You can omit the " is repeated in macro expansion [misc-macro-repeated-side-effects]" part from all check lines but the first one to make the tests more readable.
will do, see next patch.

http://reviews.llvm.org/D9496

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