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