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

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis marked 4 inline comments as done.
zinovy.nis 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
----------------
aaron.ballman wrote:
> 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
Thanks a lot!
Done.

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


================
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));
----------------
aaron.ballman wrote:
> 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.
Added a new test case for it.


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

https://reviews.llvm.org/D116478