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

[Alexander Kornienko]

In http://reviews.llvm.org/D9496#176202, @danielmarjamaki wrote:

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


Nice, but the results are from a different check: misc-macro-parentheses.


================
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;
----------------
danielmarjamaki wrote:
> 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?
> 
I can find some definitions of these macros, but I'm not sure they would be the same definitions you are talking about. It would be easier if you quoted the definitions here.

================
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 =
----------------
danielmarjamaki wrote:
> 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.
> 
Yes, you'll need to add a method that returns a "container" with arguments same way as in D9079:

  ArrayList<IdentifierInfo*> args() const {
    return ArrayList<IdentifierInfo*>(ArgumentList, NumArguments);
  }

================
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) {
----------------
danielmarjamaki wrote:
> 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.
> 
Phabricator says that you have committed that patch 10 days ago:

> Closed by commit rL236975: Refactor MacroInfo so range for loops can be used to iterate its tokens. (authored by danielmarjamaki)

http://reviews.llvm.org/D9496

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