[clang-tools-extra] r241245 - [clang-tidy] Enhance clang-tidy misc-macro-repeated-side-effects...
Daniel Marjamaki
daniel.marjamaki at evidente.se
Thu Jul 2 00:49:55 PDT 2015
Author: danielmarjamaki
Date: Thu Jul 2 02:49:55 2015
New Revision: 241245
URL: http://llvm.org/viewvc/llvm-project?rev=241245&view=rev
Log:
[clang-tidy] Enhance clang-tidy misc-macro-repeated-side-effects...
Enhance clang-tidy misc-macro-repeated-side-effects to handle ? and : better.
When ? is used in a macro, there are 2 possible control flow paths through the macro.
These paths are tracked separately so problems can be detected properly.
http://reviews.llvm.org/D10653
Modified:
clang-tools-extra/trunk/clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp
clang-tools-extra/trunk/test/clang-tidy/misc-repeated-side-effects-in-macro.c
Modified: clang-tools-extra/trunk/clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp?rev=241245&r1=241244&r2=241245&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp Thu Jul 2 02:49:55 2015
@@ -30,12 +30,12 @@ private:
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
+} // End of anonymous namespace.
void MacroRepeatedPPCallbacks::MacroExpands(const Token &MacroNameTok,
const MacroDefinition &MD,
@@ -51,10 +51,10 @@ void MacroRepeatedPPCallbacks::MacroExpa
// 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;
@@ -62,8 +62,8 @@ void MacroRepeatedPPCallbacks::MacroExpa
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 @@ void MacroRepeatedPPCallbacks::MacroExpa
}
}
-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 of __builtin_constant_p(x) is 0 if x is a macro argument
+ // with side effects. If we see a __builtin_constant_p(x) followed by a
+ // "?" "&&" or "||", then we need to reason about control flow to report
+ // warnings correctly. Until such reasoning is added, bail out 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 @@ unsigned MacroRepeatedPPCallbacks::Count
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, don't
+ // count arguments 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 @@ unsigned MacroRepeatedPPCallbacks::Count
}
// 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))
Modified: clang-tools-extra/trunk/test/clang-tidy/misc-repeated-side-effects-in-macro.c
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/misc-repeated-side-effects-in-macro.c?rev=241245&r1=241244&r2=241245&view=diff
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/misc-repeated-side-effects-in-macro.c (original)
+++ clang-tools-extra/trunk/test/clang-tidy/misc-repeated-side-effects-in-macro.c Thu Jul 2 02:49:55 2015
@@ -22,6 +22,21 @@ void bad(int ret, int a, int b) {
}
+#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 @@ void large(int a) {
// 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++);
}
More information about the cfe-commits
mailing list