I have refactored my patch.
- I added more comments.
- now the handling of __builtin_constant_p is more explicit.
- I changed uppercase=>lowercase in two functionnames in accordance with the
llvm style guide.
I have rerun the patch on various projects to make sure that the more explicit
handling of builtins don't cause false positives.
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: 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}
@@ -59,26 +74,29 @@
// CHECK-MESSAGES: :[[@LINE-1]]:70: warning: side effects in the 21st macro argument 'u'
}
-// Builtins and macros should not be counted.
-#define builtinA(x) (__builtin_constant_p(x) + (x))
-#define builtinB(x) (__builtin_constant_p(x) + (x) + (x))
-#define macroA(x) (builtinA(x) + (x))
-#define macroB(x) (builtinA(x) + (x) + (x))
+// Passing macro argument as argument to __builtin_constant_p and macros.
+#define builtinbad(x) (__builtin_constant_p(x) + (x) + (x))
+#define builtingood1(x) (__builtin_constant_p(x) + (x))
+#define builtingood2(x) ((__builtin_constant_p(x) && (x)) || (x))
+#define macrobad(x) (builtingood1(x) + (x) + (x))
+#define macrogood(x) (builtingood1(x) + (x))
void builtins(int ret, int a) {
- ret += builtinA(a++);
- ret += builtinB(a++);
+ ret += builtinbad(a++);
+ // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: side effects in the 1st macro argument 'x'
+
+ ret += builtingood1(a++);
+ ret += builtingood2(a++);
+
+ ret += macrobad(a++);
// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: side effects in the 1st macro argument 'x'
- ret += macroA(a++);
- ret += macroB(a++);
- // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: side effects in the 1st macro argument 'x'
+
+ ret += macrogood(a++);
}
// 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++);
}
Index: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp
===================================================================
--- clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp
+++ clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp
@@ -30,10 +30,10 @@
ClangTidyCheck &Check;
Preprocessor &PP;
- unsigned CountArgumentExpansions(const MacroInfo *MI,
+ unsigned countArgumentExpansions(const MacroInfo *MI,
const IdentifierInfo *Arg) const;
- bool HasSideEffects(const Token *ResultArgToks) const;
+ bool hasSideEffects(const Token *ResultArgToks) const;
};
} // namespace
@@ -51,19 +51,19 @@
// 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;
for (unsigned ArgNo = 0U; ArgNo < MI->getNumArgs(); ++ArgNo) {
const IdentifierInfo *Arg = *(MI->arg_begin() + ArgNo);
const Token *ResultArgToks = Args->getUnexpArgument(ArgNo);
- if (HasSideEffects(ResultArgToks) &&
- CountArgumentExpansions(MI, Arg) >= 2) {
+ if (hasSideEffects(ResultArgToks) &&
+ countArgumentExpansions(MI, Arg) >= 2) {
Check.diag(ResultArgToks->getLocation(),
"side effects in the %ordinal0 macro argument '%1' are "
"repeated in macro expansion")
@@ -75,12 +75,39 @@
}
}
-unsigned MacroRepeatedPPCallbacks::CountArgumentExpansions(
+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;
+ // Has a __builtin_constant_p been found?
+ bool FoundBuiltin = false;
+ // Count when "?" is reached. The "Current" will get this value when the ":"
+ // is reached.
+ std::stack<unsigned, SmallVector<unsigned, 8>> CountAtQuestion;
for (const auto &T : MI->tokens()) {
+ // The result from __builtin_constant_p(x) is 0 if x is a macro argument
+ // with side effects. If we have seen a __builtin_constant_p(x) and then
+ // there is a "?" "&&" or "||" then we need to reason about control flow
+ // to report warnings correctly. Until such reasoning is added, bailout
+ // when this happens.
+ if (FoundBuiltin && T.isOneOf(tok::question, tok::ampamp, tok::pipepipe))
+ return Max;
+
+ // Handling of ? and :.
+ if (T.is(tok::question)) {
+ 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))
@@ -97,9 +124,11 @@
if (TII == nullptr)
continue;
- // If a builtin is found within the macro definition, skip next
- // parenthesis.
- if (TII->getBuiltinID() != 0) {
+ // If a __builtin_constant_p is found within the macro definition, dont
+ // count argument inside the parentheses and remember that it has been seen
+ // in case there are "?", "&&" or "||" operators later.
+ if (TII->getBuiltinID() == Builtin::BI__builtin_constant_p) {
+ FoundBuiltin = true;
SkipParen = true;
continue;
}
@@ -114,13 +143,16 @@
}
// Count argument.
- if (TII == Arg)
- CountInMacro++;
+ if (TII == Arg) {
+ Current++;
+ if (Current > Max)
+ Max = Current;
+ }
}
- return CountInMacro;
+ return Max;
}
-bool MacroRepeatedPPCallbacks::HasSideEffects(
+bool MacroRepeatedPPCallbacks::hasSideEffects(
const Token *ResultArgToks) const {
for (; ResultArgToks->isNot(tok::eof); ++ResultArgToks) {
if (ResultArgToks->isOneOf(tok::plusplus, tok::minusminus))
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
