[clang-tidy] [bugprone-assert-side-effect] Ignore list for functions/methods

A semicolon-separated list of the names of functions or methods to be considered as not having side-effects was added for bugprone-assert-side-effect. It can be used to exclude methods like iterator::begin/end from being considered as having side-effects.

Differential Revision: https://reviews.llvm.org/D116478

clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp

@@ -7,6 +7,8 @@
 //===----------------------------------------------------------------------===//
 
 #include "AssertSideEffectCheck.h"
+#include "../utils/Matchers.h"
+#include "../utils/OptionsUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Frontend/CompilerInstance.h"
@@ -25,7 +27,9 @@ namespace bugprone {
 
 namespace {
 
-AST_MATCHER_P(Expr, hasSideEffect, bool, CheckFunctionCalls) {
+AST_MATCHER_P2(Expr, hasSideEffect, bool, CheckFunctionCalls,
+               clang::ast_matchers::internal::Matcher<NamedDecl>,
+               IgnoredFunctionsMatcher) {
   const Expr *E = &Node;
 
   if (const auto *Op = dyn_cast<UnaryOperator>(E)) {
@@ -55,7 +59,8 @@ AST_MATCHER_P(Expr, hasSideEffect, bool, CheckFunctionCalls) {
     bool Result = CheckFunctionCalls;
     if (const auto *FuncDecl = CExpr->getDirectCallee()) {
       if (FuncDecl->getDeclName().isIdentifier() &&
-          FuncDecl->getName() == "__builtin_expect") // exceptions come here
+          IgnoredFunctionsMatcher.matches(*FuncDecl, Finder,
+                                          Builder)) // exceptions come here
         Result = false;
       else if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(FuncDecl))
         Result &= !MethodDecl->isConst();
@@ -72,8 +77,9 @@ AssertSideEffectCheck::AssertSideEffectCheck(StringRef Name,
                                              ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       CheckFunctionCalls(Options.get("CheckFunctionCalls", false)),
-      RawAssertList(Options.get("AssertMacros",
-                                "assert,NSAssert,NSCAssert")) {
+      RawAssertList(Options.get("AssertMacros", "assert,NSAssert,NSCAssert")),
+      IgnoredFunctions(utils::options::parseStringList(
+          "__builtin_expect;" + Options.get("IgnoredFunctions", ""))) {
   StringRef(RawAssertList).split(AssertMacros, ",", -1, false);
 }
 
@@ -81,11 +87,17 @@ AssertSideEffectCheck::AssertSideEffectCheck(StringRef Name,
 void AssertSideEffectCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "CheckFunctionCalls", CheckFunctionCalls);
   Options.store(Opts, "AssertMacros", RawAssertList);
+  Options.store(Opts, "IgnoredFunctions",
+                utils::options::serializeStringList(IgnoredFunctions));
 }
 
 void AssertSideEffectCheck::registerMatchers(MatchFinder *Finder) {
+  auto IgnoredFunctionsMatcher =
+      matchers::matchesAnyListedName(IgnoredFunctions);
+
   auto DescendantWithSideEffect =
-      traverse(TK_AsIs, hasDescendant(expr(hasSideEffect(CheckFunctionCalls))));
+      traverse(TK_AsIs, hasDescendant(expr(hasSideEffect(
+                            CheckFunctionCalls, IgnoredFunctionsMatcher))));
   auto ConditionWithSideEffect = hasCondition(DescendantWithSideEffect);
   Finder->addMatcher(
       stmt(

clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.h

@@ -42,6 +42,7 @@ private:
   const bool CheckFunctionCalls;
   const std::string RawAssertList;
   SmallVector<StringRef, 5> AssertMacros;
+  const std::vector<std::string> IgnoredFunctions;
 };
 
 } // namespace bugprone

clang-tools-extra/docs/ReleaseNotes.rst

@@ -133,7 +133,7 @@ New checks
 - New :doc:`readability-duplicate-include
   <clang-tidy/checks/readability-duplicate-include>` check.
 
-  Looks for duplicate includes and removes them.  
+  Looks for duplicate includes and removes them.
 
 - New :doc:`readability-identifier-length
   <clang-tidy/checks/readability-identifier-length>` check.
@@ -167,7 +167,13 @@ New check aliases
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+- :doc:`bugprone-assert-side-effect <clang-tidy/checks/bugprone-assert-side-effect>`
+  check now supports an ``IgnoredFunctions`` option to explicitly consider
+  the specified semicolon-separated functions list as not having any
+  side-effects. Regular expressions for the list items are also accepted.
+
 - Removed default setting ``cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors = "true"``,
+  from :doc:`cppcoreguidelines-explicit-virtual-functions <clang-tidy/checks/cppcoreguidelines-explicit-virtual-functions>`
   to match the current state of the C++ Core Guidelines.
 
 - Removed suggestion ``use gsl::at`` from warning message in the
@@ -185,10 +191,10 @@ Changes in existing checks
 
 - Fixed a false positive in :doc:`bugprone-throw-keyword-missing
   <clang-tidy/checks/bugprone-throw-keyword-missing>` when creating an exception object
-  using placement new
+  using placement new.
 
 - :doc:`cppcoreguidelines-narrowing-conversions <clang-tidy/checks/cppcoreguidelines-narrowing-conversions>`
-  check now supports a `WarnOnIntegerToFloatingPointNarrowingConversion`
+  check now supports a ``WarnOnIntegerToFloatingPointNarrowingConversion``
   option to control whether to warn on narrowing integer to floating-point
   conversions.
 

clang-tools-extra/docs/clang-tidy/checks/bugprone-assert-side-effect.rst

@@ -21,3 +21,13 @@ Options
    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,
+   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`).

clang-tools-extra/test/clang-tidy/checkers/bugprone-assert-side-effect.cpp

@@ -1,4 +1,4 @@
-// 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'}]}" -- -fexceptions
+// 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; }
@@ -43,9 +43,12 @@ void print(...);
 
 //===----------------------------------------------------------------------===//
 
+bool badButIgnoredFunc(int a, int b) { return a * b > 0; }
+
 class MyClass {
 public:
   bool badFunc(int a, int b) { return a * b > 0; }
+  bool badButIgnoredFunc(int a, int b) { return a * b > 0; }
   bool goodFunc(int a, int b) const { return a * b > 0; }
 
   MyClass &operator=(const MyClass &rhs) { return *this; }
@@ -57,6 +60,11 @@ public:
   void operator delete(void *p) {}
 };
 
+class SomeoneElseClass {
+public:
+  bool badButIgnoredFunc(int a, int b) { return a * b > 0; }
+};
+
 bool freeFunction() {
   return true;
 }
@@ -85,8 +93,16 @@ int main() {
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in assert() condition discarded in release builds
 
   MyClass mc;
+  SomeoneElseClass sec;
   assert(mc.badFunc(0, 1));
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in assert() condition discarded in release builds
+  assert(mc.badButIgnoredFunc(0, 1));
+  // badButIgnoredFunc is not ignored as only class members are ignored by the config
+  assert(badButIgnoredFunc(0, 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in assert() condition discarded in release builds
+  // sec.badButIgnoredFunc is not ignored as only MyClass members are ignored by the config
+  assert(sec.badButIgnoredFunc(0, 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in assert() condition discarded in release builds
   assert(mc.goodFunc(0, 1));
 
   MyClass mc2;