Hi alexfh,
This patch fixes possible false negatives when macro contains ?:.
I tested this improvemed checker on 1398 open source projects, but did not see
any new warnings.. At least that means that it did not cause any new false
positives even though it does fix the false negatives that are shown in the
testing.
Question: Is it a good idea to use std::stack or would some other container be
better in clang-tidy?
http://reviews.llvm.org/D10653
Files:
clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp
test/clang-tidy/misc-repeated-side-effects-in-macro.c
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
Index: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp
===================================================================
--- clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp
+++ clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp
@@ -51,10 +51,10 @@
// making the macro too complex.
if (std::find_if(
MI->tokens().begin(), MI->tokens().end(), [](const Token &T) {
- return 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);
+ return T.isOneOf(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);
}) != MI->tokens().end())
return;
@@ -77,10 +77,30 @@
unsigned MacroRepeatedPPCallbacks::CountArgumentExpansions(
const MacroInfo *MI, const IdentifierInfo *Arg) const {
- unsigned CountInMacro = 0;
+ // Current argument count. When moving forward to a different control-flow
+ // path this can decrease.
+ unsigned Current = 0;
+ // Max argument count.
+ unsigned Max = 0;
bool SkipParen = false;
int SkipParenCount = 0;
+ bool FoundBuiltin = false;
+ std::stack<unsigned> CountAtQuestion;
for (const auto &T : MI->tokens()) {
+ // Handling of ? and :.
+ if (T.is(tok::question)) {
+ // Result from __builtin might be known, maybe only the true or false
+ // expression should be analysed.
+ if (FoundBuiltin)
+ return Max;
+ CountAtQuestion.push(Current);
+ } else if (T.is(tok::colon)) {
+ if (CountAtQuestion.empty())
+ return 0;
+ Current = CountAtQuestion.top();
+ CountAtQuestion.pop();
+ }
+
// If current token is a parenthesis, skip it.
if (SkipParen) {
if (T.is(tok::l_paren))
@@ -100,6 +120,7 @@
// If a builtin is found within the macro definition, skip next
// parenthesis.
if (TII->getBuiltinID() != 0) {
+ FoundBuiltin = true;
SkipParen = true;
continue;
}
@@ -114,10 +135,13 @@
}
// Count argument.
- if (TII == Arg)
- CountInMacro++;
+ if (TII == Arg) {
+ Current++;
+ if (Current > Max)
+ Max = Current;
+ }
}
- return CountInMacro;
+ return Max;
}
bool MacroRepeatedPPCallbacks::HasSideEffects(
Index: test/clang-tidy/misc-repeated-side-effects-in-macro.c
===================================================================
--- test/clang-tidy/misc-repeated-side-effects-in-macro.c
+++ test/clang-tidy/misc-repeated-side-effects-in-macro.c
@@ -22,6 +22,21 @@
}
+#define MIN(A,B) ((A) < (B) ? (A) : (B)) // single ?:
+#define LIMIT(X,A,B) ((X) < (A) ? (A) : ((X) > (B) ? (B) : (X))) // two ?:
+void question(int x) {
+ MIN(x++, 12);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: side effects in the 1st macro argument 'A'
+ MIN(34, x++);
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: side effects in the 2nd macro argument 'B'
+ LIMIT(x++, 0, 100);
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: side effects in the 1st macro argument 'X'
+ LIMIT(20, x++, 100);
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: side effects in the 2nd macro argument 'A'
+ LIMIT(20, 0, x++);
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: side effects in the 3rd macro argument 'B'
+}
+
// 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}
@@ -74,11 +89,9 @@
}
// Bail out for conditionals.
-#define condA(x,y) ((x)>(y)?(x):(y))
#define condB(x,y) if(x) {x=y;} else {x=y + 1;}
-void conditionals(int ret, int a, int b)
+void conditionals(int a, int b)
{
- ret += condA(a++, b++);
condB(a, b++);
}
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
