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,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; @@ -62,8 +62,8 @@ 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> 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)) 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++); }