[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 cfe-commits at lists.llvm.org
Thu Jan 20 12:17:24 PST 2022

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:


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.



More information about the cfe-commits mailing list