[PATCH] D116478: [clang-tidy] A semicolon-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
----------------
zinovy.nis wrote:
> 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?
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.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.h:13
 #include "../ClangTidyCheck.h"
+#include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/SmallVector.h"
----------------
I don't think this included is needed?


================
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.
+
----------------
aaron.ballman wrote:
> 
I think this comment is still not done yet.


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

https://reviews.llvm.org/D116478