[clang-tidy] A semicolon-separated list of the names of functions or methods to be considered as not having side-effects

[zinovy.nis]

Summary

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.

[zinovy.nis]

Fix clang-format issues.

[Eugene.Zelenko]

Please mention new option in Release Notes (in changes in existing checks section).

[zinovy.nis]

Updated release notes.

[zinovy.nis]

In D116478#3216082, @Eugene.Zelenko wrote:

Please mention new option in Release Notes (in changes in existing checks section).

Done

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

169

Please separate with newline and use single back-ticks for options.

175–176

Ditto for back-ticks.

187

Ditto for back-ticks.

[carlosgalvezp]

clang-tools-extra/docs/ReleaseNotes.rst

169

I introduced those double backticks due to review comments. As it turns out, single backticks are only for links, not for formatted text. Should they be brought back?

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

169

Double back-ticks are for language constructs, single back-ticks for options, tool names, etc.

[carlosgalvezp]

clang-tools-extra/docs/ReleaseNotes.rst

169

Hm, I see. I think visually it's much more helpful to have options rendered as formatted code (just like you'd see them in the .clang-tidy file in a code editor) instead of in italic, which is what is rendered with single backticks.

[carlosgalvezp]

clang-tools-extra/docs/ReleaseNotes.rst

169

Also note that the main clang-tidy doc page uses doble backticks for options, not just language constructs, so I think it would be good to keep consistency with that:

https://raw.githubusercontent.com/llvm/llvm-project/main/clang-tools-extra/docs/clang-tidy/index.rst

Single backticks are used there only for links and together with the :program: keyword.

[carlosgalvezp]

clang-tools-extra/docs/ReleaseNotes.rst

169

Unless there's some documented style convention that says otherwise, of course. I haven't found anything in the LLVM Coding Standards.

[aaron.ballman]

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

36–47

Instead of using "Exception" here, can we rename slightly? Functions can have lists of exceptions (dynamic exception specifications are a thing in C++), so the use of "exception" can be somewhat confusing, but I think something like IgnoredFunctionList and IgnoredFunctions would be an improvement.

[zinovy.nis]

• FunctionException -> IgnoredFunctions
• Fixed double ticks in rst docs.

[aaron.ballman]

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

60–64

This doesn't seem quite right to me (test coverage would help) in the case where the user is specifying a (potentially partially) qualified function name. e.g., imagine an IgnoredFunctions list of my::fancy_func,::other_func,yet::another_func where my is a namespace containing a function named fancy_func, and yet is a class with a static function named another_func. I think this code will only consider the name of the function itself, but not any part of its qualified name.

I think we typically implement function name exclusions via the matchesAnyListedName() AST matcher, as in: https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp#L92

clang-tools-extra/docs/ReleaseNotes.rst

171–172

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

27

This doesn't document whether the names can be qualified or not, or whether the names have to have an exact match instead of a regex match.

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

99

FWIW, I think this can be confusing in practice; the function called is ::MyClass:badButIgnoredFunc(). Further, if I had a global function named badButIgnoredFunc() it would *also* be ignored and I'd have no way to configure to distinguish between the two.

[zinovy.nis]

• Support regexp for list of ignored functions.

[zinovy.nis]

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

60–64

Thanks a lot!
Done.

BTW, now we have "," separator for macros and ";" for ignored functions. Doesn't sound reasonable. What do you think?

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

99

Added a new test case for it.

[zinovy.nis]

Any other comments?

[aaron.ballman]

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

60–64

Neato, it seems we have this inconsistency in a few places:

https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/openmp/ExceptionEscapeCheck.cpp#L32
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp#L484
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/cert/ProperlySeededRandomGeneratorCheck.cpp#L25
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp#L35
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp#L41
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp#L25

I agree it's an issue, but we use parseStringList() far more often than we manually split (42 times compared to 7). I think we should keep using parseStringList() and someday we can hopefully switch the remaining uses of comma separated lists to be semicolon separated.

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

13

I don't think this included is needed?

clang-tools-extra/docs/ReleaseNotes.rst

171–172

I think this comment is still not done yet.

[zinovy.nis]

• Remove unused include.
• Update release notes.

[aaron.ballman]

LGTM aside from a nit in the documentation.

clang-tools-extra/docs/ReleaseNotes.rst

172

[zinovy.nis]

clang-tools-extra/docs/ReleaseNotes.rst

172

Thanks!

Fixed and pushed.

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

 //===--- AssertSideEffectCheck.h - clang-tidy -------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ASSERTSIDEEFFECTCHECK_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ASSERTSIDEEFFECTCHECK_H
 
 #include "../ClangTidyCheck.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include <string>
 
 namespace clang {
 namespace tidy {
 namespace bugprone {
 
 /// Finds `assert()` with side effect.
 ///
 /// The condition of `assert()` is evaluated only in debug builds so a
 /// condition with side effect can cause different behavior in debug / release
 /// builds.
 ///
 /// There are two options:
 ///
 ///   - `AssertMacros`: A comma-separated list of the names of assert macros to
 ///     be checked.
 ///   - `CheckFunctionCalls`: 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.
 class AssertSideEffectCheck : public ClangTidyCheck {
 public:
   AssertSideEffectCheck(StringRef Name, ClangTidyContext *Context);
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   const bool CheckFunctionCalls;
   const std::string RawAssertList;
   SmallVector<StringRef, 5> AssertMacros;
+  const std::vector<std::string> IgnoredFunctions;
 };
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ASSERTSIDEEFFECTCHECK_H

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

 //===--- AssertSideEffectCheck.cpp - clang-tidy ---------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #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"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
 #include <algorithm>
 #include <string>
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 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)) {
     UnaryOperator::Opcode OC = Op->getOpcode();
     return OC == UO_PostInc || OC == UO_PostDec || OC == UO_PreInc ||
            OC == UO_PreDec;
   }
 
return OpKind == OO_Equal || OpKind == OO_PlusEqual ||
            OpKind == OO_PlusPlus || OpKind == OO_MinusMinus ||
            OpKind == OO_PercentEqual || OpKind == OO_New ||
            OpKind == OO_Delete || OpKind == OO_Array_New ||
            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() &&
-          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();
     }
     return Result;
   }
 
   return isa<CXXNewExpr>(E) || isa<CXXDeleteExpr>(E) || isa<CXXThrowExpr>(E);
 }
 
 } // namespace
 
 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);
 }
 
 // The options are explained in AssertSideEffectCheck.h.
 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(
           anyOf(conditionalOperator(ConditionWithSideEffect),
                 ifStmt(ConditionWithSideEffect),
                 unaryOperator(hasOperatorName("!"),
                               hasUnaryOperand(unaryOperator(
                                   hasOperatorName("!"),

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`readability-container-data-pointer
   <clang-tidy/checks/readability-container-data-pointer>` check.
 
   Finds cases where code could use ``data()`` rather than the address of the
   element at index 0 in a container.
 
 - 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.
 
   Reports identifiers whose names are too short. Currently checks local
   variables and function parameters only.
 
 - New :doc:`misc-misleading-bidirectional <clang-tidy/checks/misc-misleading-bidirectional>` check.

 
 - New alias :doc:`cert-flp37-c
   <clang-tidy/checks/cert-flp37-c>` to
   :doc:`bugprone-suspicious-memory-comparison
   <clang-tidy/checks/bugprone-suspicious-memory-comparison>` was added.
 
 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
   ``cppcoreguidelines-pro-bounds-constant-array-index`` check, since that is not
   a requirement from the C++ Core Guidelines. This allows people to choose
   their own safe indexing strategy. The fix-it is kept for those who want to
   use the GSL library.
 
 - Updated :doc:`google-readability-casting
   <clang-tidy/checks/google-readability-casting>` to diagnose and fix functional
   casts, to achieve feature parity with the corresponding ``cpplint.py`` check.
 
 - Fixed a false positive in :doc:`fuchsia-trailing-return
   <clang-tidy/checks/fuchsia-trailing-return>` for C++17 deduction guides.
 
 - 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.
 
 - Improved :doc:`performance-move-const-arg <clang-tidy/checks/performance-move-const-arg>` check.
 
   Removed a wrong FixIt for trivially copyable objects wrapped by ``std::move()`` and passed to an rvalue reference parameter. Removal of ``std::move()`` would break the code.
 
 Removed checks

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

.. option:: AssertMacros
 
    A comma-separated list of the names of assert macros to be checked.
 
 .. option:: CheckFunctionCalls
 
    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

-// 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; }
 
 #ifdef NDEBUG
 #define assert(x) 1
 #else
 #define assert(x)                                                              \

 #define msvc_assert(expression) (void)(                                        \
             (!!(expression)) ||                                                \
             (abort(), 0)                                                       \
         )
 
 
 //===----------------------------------------------------------------------===//
 
+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; }
 
   int operator-() { return 1; }
 
   operator bool() const { return true; }
 
   void operator delete(void *p) {}
 };
 
+class SomeoneElseClass {
+public:
+  bool badButIgnoredFunc(int a, int b) { return a * b > 0; }
+};
+
 bool freeFunction() {
   return true;
 }
 
 int main() {
 
   int X = 0;
   bool B = false;
int main() {
   assert(!B);
 
   assert(B || true);
 
   assert(freeFunction());
   // 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;
   assert(mc2 = mc);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in assert() condition discarded in release builds
 
   assert(-mc > 0);