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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp:57-60
     if (const auto *FuncDecl = CExpr->getDirectCallee()) {
       if (FuncDecl->getDeclName().isIdentifier() &&
-          FuncDecl->getName() == "__builtin_expect") // exceptions come here
+          IgnoredFunctions.contains(
+              FuncDecl->getName())) // exceptions come here
----------------
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


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:149-150
+- :doc:`bugprone-assert-side-effect <clang-tidy/checks/bugprone-assert-side-effect>`
+  check now supports a ``IgnoredFunctions`` option to explicitly consider the specified
+  functions or methods as not any having side-effects.
+
----------------



================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-assert-side-effect.rst:27
+
+   A comma-separated list of the names of functions or methods to be
+   considered as not having side-effects.
----------------
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.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-assert-side-effect.cpp:91
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in assert() condition discarded in release builds
+  assert(mc.badButIgnoredFunc(0, 1));
   assert(mc.goodFunc(0, 1));
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116478/new/

https://reviews.llvm.org/D116478