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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
