PiotrZSL created this revision.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a reviewer: njames93.
Herald added a project: All.
PiotrZSL requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
- Fixed problem when after changes in macros handling in Clang-tidy 14 cased
issues from from this check be hidden when assert is defined in system headers.
Now raising diagnostic for a place from where macro is called.
- Added support for const, pure, constexpr functions/methods. Now they
considered safe.
- Split configuration options CheckFunctionCalls, IgnoredFunctions, to support
enabling only checking of methods, or only of functions.
Fixes: #47605, #25569, #31821
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D147081
Files:
clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp
clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone/assert-side-effect.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/assert-side-effect/assert.h
clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect-constexpr.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp
+++ 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 <assert.h>
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;
}
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect-constexpr.cpp
===================================================================
--- /dev/null
+++ 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 <assert.h>
+
+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;
+}
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/assert-side-effect/assert.h
===================================================================
--- /dev/null
+++ 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) \
+ )
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/assert-side-effect.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/bugprone/assert-side-effect.rst
+++ 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
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -153,6 +153,15 @@
Changes in existing checks
^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+- The :doc:`bugprone-assert-side-effect
+ <clang-tidy/checks/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
<clang-tidy/checks/readability/redundant-string-cstr>` check to recognise
unnecessary ``std::string::c_str()`` and ``std::string::data()`` calls in
Index: clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.h
+++ clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.h
@@ -38,9 +38,11 @@
private:
const bool CheckFunctionCalls;
+ const bool CheckMethodCalls;
const StringRef RawAssertList;
SmallVector<StringRef, 5> AssertMacros;
const std::vector<StringRef> IgnoredFunctions;
+ const std::vector<StringRef> IgnoredMethods;
};
} // namespace clang::tidy::bugprone
Index: clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp
+++ 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<NamedDecl>,
- IgnoredFunctionsMatcher) {
+ IgnoredDeclMatcher) {
const Expr *E = &Node;
if (const auto *Op = dyn_cast<UnaryOperator>(E)) {
@@ -53,50 +53,67 @@
OpKind == OO_Array_Delete;
}
- if (const auto *CExpr = dyn_cast<CallExpr>(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<CXXMethodDecl>(FuncDecl))
- Result &= !MethodDecl->isConst();
- }
- return Result;
- }
+ if (isa<CXXNewExpr>(E) || isa<CXXDeleteExpr>(E) || isa<CXXThrowExpr>(E))
+ return true;
+
+ if (!CheckCalls)
+ return false;
+
+ const auto *CExpr = dyn_cast<CallExpr>(E);
+ if (!CExpr)
+ return false;
+
+ const auto *FuncDecl = CExpr->getDirectCallee();
+ if (!FuncDecl || !FuncDecl->getDeclName().isIdentifier())
+ return false;
- return isa<CXXNewExpr>(E) || isa<CXXDeleteExpr>(E) || isa<CXXThrowExpr>(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;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
