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

[Daniel Marjamäki]

Here are some results:
https://drive.google.com/file/d/0BykPmWrCOxt2YTdOU0Y3M1RmdGM/view?usp=sharing


================
Comment at: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp:49
@@ +48,3 @@
+  // Don't write warnings for macros defined in system headers
+  if (SM.isInSystemHeader(MI->getDefinitionLoc()))
+    return;
----------------
alexfh wrote:
> What system macros did get flagged here that you wouldn't want? Combining MIN/MAX with a side effect seems like a bad idea, for example.
For instance: toupper, strchr. I think these were false positives. :-(

For instance for strchr I assume "__builtin_constant_p (s)" does not use s at runtime and then it's safe to use s again later in the macro.

I don't see how we can teach the checker to not care about any compiletime behaviour. do you know if this is possible?


================
Comment at: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp:53
@@ +52,3 @@
+  typedef MacroInfo::arg_iterator ArgIter;
+  for (ArgIter AI = MI->arg_begin(), AE = MI->arg_end(); AI != AE; ++AI) {
+    const Token *ResultArgToks =
----------------
alexfh wrote:
> So, will a range-based for loop work here?
> 
> Also, when I send patches for review, I find it convenient to answer each reviewer's comment with "Done" or a reason why the comment is not applicable. This helps keeping track of the issues I addressed.
How?

Then I must modify MacroInfo right? Either the arguments must be stored in some container. Or else MacroInfo can have a method that creates a container with the arguments and returns it, I know this is not a good solution but seems easier for me to implement ;-).

I would prefer to not change MacroInfo in this patch.. but I could submit a parallell patch.


================
Comment at: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp:84
@@ +83,3 @@
+            ResultArgToks->getLocation(),
+            "side effects in macro argument is repeated in macro expansion");
+        Check.diag(MI->getDefinitionLoc(), "%0 macro defined here",
----------------
alexfh wrote:
> I'd make the message more specific by adding at least the index of the argument that is repeated. Maybe also add the name of the parameter in the note.
Yes I agree.. will do.

================
Comment at: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp:50
@@ +49,3 @@
+  typedef clang::MacroInfo::arg_iterator ArgIter;
+  for (ArgIter AI = MI->arg_begin(), AE = MI->arg_end(); AI != AE; ++AI) {
+    const clang::Token *ResultArgToks =
----------------
alexfh wrote:
> Will a range-based for loop work here?
I don't see how I can do this without modifying the MacroInfo, I'd need a new method that returns a container with the arguments right? Maybe the MacroInfo::ArgumentList should be a container? 

I would prefer to do such refactoring separately.


================
Comment at: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp:54
@@ +53,3 @@
+    int CountInMacro = 0;
+    for (TokIter TI = MI->tokens_begin(), TE = MI->tokens_end(); TI != TE;
+         ++TI) {
----------------
alexfh wrote:
> ditto
Impossible.

Fortunately I have a patch pending (http://reviews.llvm.org/D9079) that makes it possible to do this. If it gets the OK and I can apply it I will of course use the range-based for loop here too.

http://reviews.llvm.org/D9496

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