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/
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
