diff --git a/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.h b/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.h --- a/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.h @@ -38,9 +38,11 @@ private: const bool CheckFunctionCalls; + const bool CheckMethodCalls; const StringRef RawAssertList; SmallVector AssertMacros; const std::vector IgnoredFunctions; + const std::vector IgnoredMethods; }; } // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp --- a/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp @@ -25,9 +25,9 @@ namespace { -AST_MATCHER_P2(Expr, hasSideEffect, bool, CheckFunctionCalls, +AST_MATCHER_P2(Expr, hasSideEffect, bool, CheckCalls, clang::ast_matchers::internal::Matcher, - IgnoredFunctionsMatcher) { + IgnoredDeclMatcher) { const Expr *E = &Node; if (const auto *Op = dyn_cast(E)) { @@ -53,50 +53,67 @@ OpKind == OO_Array_Delete; } - if (const auto *CExpr = dyn_cast(E)) { - bool Result = CheckFunctionCalls; - if (const auto *FuncDecl = CExpr->getDirectCallee()) { - if (FuncDecl->getDeclName().isIdentifier() && - IgnoredFunctionsMatcher.matches(*FuncDecl, Finder, - Builder)) // exceptions come here - Result = false; - else if (const auto *MethodDecl = dyn_cast(FuncDecl)) - Result &= !MethodDecl->isConst(); - } - return Result; - } + if (isa(E) || isa(E) || isa(E)) + return true; + + if (!CheckCalls) + return false; + + const auto *CExpr = dyn_cast(E); + if (!CExpr) + return false; + + const FunctionDecl *FuncDecl = CExpr->getDirectCallee(); + if (!FuncDecl || !FuncDecl->getDeclName().isIdentifier()) + return false; - return isa(E) || isa(E) || isa(E); + return !IgnoredDeclMatcher.matches(*FuncDecl, Finder, Builder); } +AST_MATCHER_P(Decl, isEnabled, bool, Value) { return Value; } + } // namespace AssertSideEffectCheck::AssertSideEffectCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), CheckFunctionCalls(Options.get("CheckFunctionCalls", false)), + CheckMethodCalls(Options.get("CheckMethodCalls", false)), RawAssertList(Options.get("AssertMacros", "assert,NSAssert,NSCAssert")), IgnoredFunctions(utils::options::parseListPair( - "__builtin_expect;", Options.get("IgnoredFunctions", ""))) { + "__builtin_expect;", Options.get("IgnoredFunctions", ""))), + IgnoredMethods( + utils::options::parseStringList(Options.get("IgnoredMethods", ""))) { StringRef(RawAssertList).split(AssertMacros, ",", -1, false); } // The options are explained in AssertSideEffectCheck.h. void AssertSideEffectCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "CheckFunctionCalls", CheckFunctionCalls); + Options.store(Opts, "CheckMethodCalls", CheckMethodCalls); Options.store(Opts, "AssertMacros", RawAssertList); Options.store(Opts, "IgnoredFunctions", utils::options::serializeStringList(IgnoredFunctions)); + Options.store(Opts, "IgnoredMethods", + utils::options::serializeStringList(IgnoredMethods)); } void AssertSideEffectCheck::registerMatchers(MatchFinder *Finder) { - auto IgnoredFunctionsMatcher = - matchers::matchesAnyListedName(IgnoredFunctions); - - auto DescendantWithSideEffect = - traverse(TK_AsIs, hasDescendant(expr(hasSideEffect( - CheckFunctionCalls, IgnoredFunctionsMatcher)))); + auto IgnoredMatcher = functionDecl( + anyOf( + functionDecl(isEnabled(CheckFunctionCalls), unless(cxxMethodDecl())), + cxxMethodDecl(isEnabled(CheckMethodCalls))), + anyOf(hasAttr(clang::attr::Const), hasAttr(clang::attr::Pure), + isConstexpr(), + namedDecl(unless(cxxMethodDecl()), + matchers::matchesAnyListedName(IgnoredFunctions)), + cxxMethodDecl(anyOf( + isConst(), matchers::matchesAnyListedName(IgnoredMethods))))); + auto DescendantWithSideEffect = traverse( + TK_AsIs, hasDescendant(expr(hasSideEffect( + CheckFunctionCalls || CheckMethodCalls, IgnoredMatcher)))); auto ConditionWithSideEffect = hasCondition(DescendantWithSideEffect); + Finder->addMatcher( stmt( anyOf(conditionalOperator(ConditionWithSideEffect), @@ -117,13 +134,13 @@ StringRef AssertMacroName; while (Loc.isValid() && Loc.isMacroID()) { StringRef MacroName = Lexer::getImmediateMacroName(Loc, SM, LangOpts); + Loc = SM.getImmediateMacroCallerLoc(Loc); // Check if this macro is an assert. if (llvm::is_contained(AssertMacros, MacroName)) { AssertMacroName = MacroName; break; } - Loc = SM.getImmediateMacroCallerLoc(Loc); } if (AssertMacroName.empty()) return; diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -153,6 +153,15 @@ Changes in existing checks ^^^^^^^^^^^^^^^^^^^^^^^^^^ + +- The :doc:`bugprone-assert-side-effect + ` check now supports + function attributes ``pure/const``, and ``constexpr`` functions. Configuration + options `CheckFunctionCalls` and `IgnoredFunctions` have been split into + separate options for functions and methods. Additionally, a bug was fixed + that previously required using the `--system-headers` switch to see issues + related to the usage of macros defined in system headers. + - Improved :doc:`readability-redundant-string-cstr ` check to recognise unnecessary ``std::string::c_str()`` and ``std::string::data()`` calls in diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/assert-side-effect.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/assert-side-effect.rst --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/assert-side-effect.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/assert-side-effect.rst @@ -9,6 +9,9 @@ condition with side effect can cause different behavior in debug / release builds. +Functions and methods marked with the attributes ``pure``, ``const``, or +``constexpr`` (C++11) are not treated as potentially producing side-effects. + Options ------- @@ -18,14 +21,30 @@ .. option:: CheckFunctionCalls + Whether to treat non-member functions as they produce side effects. + Disabled by default because it can increase the number of false + positive warnings. + +.. option:: CheckiMethodCalls + Whether to treat non-const member and non-member functions as they produce side effects. Disabled by default because it can increase the number of false positive warnings. .. option:: IgnoredFunctions - A semicolon-separated list of the names of functions or methods to be - considered as not having side-effects. Regular expressions are accepted, + A semicolon-separated list of the names of functions to be considered as + not having side-effects. Regular expressions are accepted, + e.g. `[Rr]ef(erence)?$` matches every type with suffix `Ref`, `ref`, + `Reference` and `reference`. The default is empty. If a name in the list + contains the sequence `::` it is matched against the qualified typename + (i.e. `namespace::Type`, otherwise it is matched against only + the type name (i.e. `Type`). + +.. option:: IgnoredMethods + + A semicolon-separated list of the names of methods to be considered as + not having side-effects. Regular expressions are accepted, e.g. `[Rr]ef(erence)?$` matches every type with suffix `Ref`, `ref`, `Reference` and `reference`. The default is empty. If a name in the list contains the sequence `::` it is matched against the qualified typename diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/assert-side-effect/assert.h b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/assert-side-effect/assert.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/assert-side-effect/assert.h @@ -0,0 +1,40 @@ +#pragma once + +int abort() { return 0; } + +#ifdef NDEBUG +#define assert(x) 1 +#else +#define assert(x) \ + if (!(x)) \ + (void)abort() +#endif + +void print(...); +#define assert2(e) (__builtin_expect(!(e), 0) ? \ + print (#e, __FILE__, __LINE__) : (void)0) + +#ifdef NDEBUG +#define my_assert(x) 1 +#else +#define my_assert(x) \ + ((void)((x) ? 1 : abort())) +#endif + +#ifdef NDEBUG +#define not_my_assert(x) 1 +#else +#define not_my_assert(x) \ + if (!(x)) \ + (void)abort() +#endif + +#define real_assert(x) ((void)((x) ? 1 : abort())) +#define wrap1(x) real_assert(x) +#define wrap2(x) wrap1(x) +#define convoluted_assert(x) wrap2(x) + +#define msvc_assert(expression) (void)( \ + (!!(expression)) || \ + (abort(), 0) \ + ) diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect-constexpr.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect-constexpr.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect-constexpr.cpp @@ -0,0 +1,34 @@ +// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-assert-side-effect %t -- \ +// RUN: -config="{CheckOptions: [{key: bugprone-assert-side-effect.CheckFunctionCalls, value: true}, \ +// RUN: {key: bugprone-assert-side-effect.CheckMethodCalls, value: true}, \ +// RUN: {key: bugprone-assert-side-effect.AssertMacros, value: 'assert'}]}" \ +// RUN: -- -fexceptions -isystem %S/Inputs/assert-side-effect + +#include + +class MyClass { +public: + bool badFunc(int a, int b) { return a * b > 0; } + constexpr bool goodFunc(int a, int b) { return a * b > 0; } +}; + +bool badFunction() { + return false; +} + +constexpr bool goodFunction() { + return true; +} + +int main() { + + MyClass mc; + assert(mc.badFunc(0, 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in assert() condition discarded in release builds + assert(badFunction()); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in assert() condition discarded in release builds + + assert(mc.goodFunc(0, 1)); + assert(goodFunction); + return 0; +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp @@ -1,47 +1,12 @@ -// RUN: %check_clang_tidy %s bugprone-assert-side-effect %t -- -config="{CheckOptions: [{key: bugprone-assert-side-effect.CheckFunctionCalls, value: true}, {key: bugprone-assert-side-effect.AssertMacros, value: 'assert,assert2,my_assert,convoluted_assert,msvc_assert'}, {key: bugprone-assert-side-effect.IgnoredFunctions, value: 'MyClass::badButIgnoredFunc'}]}" -- -fexceptions - -//===--- assert definition block ------------------------------------------===// -int abort() { return 0; } - -#ifdef NDEBUG -#define assert(x) 1 -#else -#define assert(x) \ - if (!(x)) \ - (void)abort() -#endif - -void print(...); -#define assert2(e) (__builtin_expect(!(e), 0) ? \ - print (#e, __FILE__, __LINE__) : (void)0) - -#ifdef NDEBUG -#define my_assert(x) 1 -#else -#define my_assert(x) \ - ((void)((x) ? 1 : abort())) -#endif - -#ifdef NDEBUG -#define not_my_assert(x) 1 -#else -#define not_my_assert(x) \ - if (!(x)) \ - (void)abort() -#endif - -#define real_assert(x) ((void)((x) ? 1 : abort())) -#define wrap1(x) real_assert(x) -#define wrap2(x) wrap1(x) -#define convoluted_assert(x) wrap2(x) - -#define msvc_assert(expression) (void)( \ - (!!(expression)) || \ - (abort(), 0) \ - ) - - -//===----------------------------------------------------------------------===// +// RUN: %check_clang_tidy %s bugprone-assert-side-effect %t -- \ +// RUN: -config="{CheckOptions: [{key: bugprone-assert-side-effect.CheckFunctionCalls, value: true}, \ +// RUN: {key: bugprone-assert-side-effect.CheckMethodCalls, value: true}, \ +// RUN: {key: bugprone-assert-side-effect.AssertMacros, value: 'assert,assert2,my_assert,convoluted_assert,msvc_assert'}, \ +// RUN: {key: bugprone-assert-side-effect.IgnoredFunctions, value: 'functionToIgnore'}, \ +// RUN: {key: bugprone-assert-side-effect.IgnoredMethods, value: 'MyClass::badButIgnoredFunc'}]}" \ +// RUN: -- -fexceptions -isystem %S/Inputs/assert-side-effect + +#include bool badButIgnoredFunc(int a, int b) { return a * b > 0; } @@ -69,6 +34,22 @@ return true; } +bool functionToIgnore() { + return true; +} + +bool constFunction(bool input) __attribute__ ((const)) { + return !input; +} + +bool pureFunction(bool input) __attribute__ ((pure)) { + return !input; +} + +constexpr bool constexprFunction(bool input) { + return !input; +} + int main() { int X = 0; @@ -126,5 +107,10 @@ msvc_assert(mc2 = mc); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in msvc_assert() condition discarded in release builds + assert(functionToIgnore()); + assert(constFunction(false)); + assert(pureFunction(false)); + assert(constexprFunction(false)); + return 0; }