njames93 added a comment.
Few stylistic nits, Also Theres lots of cases where single stmt if statements
have braces, typically we elide those braces.
Is it worth flagging methods with Thread safety analysis
<https://clang.llvm.org/docs/ThreadSafetyAnalysis.html> attributes.
These are only used in clang, but if a project annotates their methods with
these, it would be nice to autodetect these attributes, though I can see this
being a big job and likely better for a follow up.
================
Comment at:
clang-tools-extra/clang-tidy/concurrency/AsyncBlockingCheck.cpp:16-21
+static const auto kName = "name";
+static const auto kType = "type";
+static const auto kFunction = "function";
+static const auto kMethod = "method";
+static const auto kAtomic = "atomic";
+static const auto kLockfree = "lockfree";
----------------
These go against llvm naming convention of Variables being CamelCase.
Also don't use auto when the type is spelled in the initialization.
================
Comment at: clang-tools-extra/clang-tidy/concurrency/AsyncBlockingCheck.cpp:23
+
+static std::vector<clang::StringRef>
+toVector(const std::vector<clang::StringRef> Base, clang::StringRef Extra) {
----------------
`clang::StringRef` is a code smell imo, use `llvm::StringRef` or move this code
into the clang namespace and drop the qualifier.
================
Comment at: clang-tools-extra/clang-tidy/concurrency/AsyncBlockingCheck.cpp:24
+static std::vector<clang::StringRef>
+toVector(const std::vector<clang::StringRef> Base, clang::StringRef Extra) {
+ llvm::SmallVector<clang::StringRef, 4> Tmp{Base.begin(), Base.end()};
----------------
This could take an ArrayRef instead of const vector ref.
================
Comment at: clang-tools-extra/clang-tidy/concurrency/AsyncBlockingCheck.cpp:33
+
+static const std::vector<clang::StringRef> LockableBase = {
+ /* C++ std */
----------------
This and all examples below don't belong on the heap, Just use an array.
================
Comment at: clang-tools-extra/clang-tidy/concurrency/AsyncBlockingCheck.cpp:35
+ /* C++ std */
+ "std::mutex", //
+ "std::timed_mutex", //
----------------
Is it wise to fully qualify these?
`::std::mutex`
Also whats with the comments at the end of each line, they don't seem to add
anything.
================
Comment at: clang-tools-extra/clang-tidy/concurrency/AsyncBlockingCheck.cpp:222
+ // std::mutex
+ auto Lockable = toVector(LockableBase, StringRef(LockableExtra));
+ Finder->addMatcher(
----------------
No need to case to `StringRef`, its implicit.
================
Comment at:
clang-tools-extra/clang-tidy/concurrency/AsyncBlockingCheck.cpp:292-293
+
+ const auto *D = Result.Nodes.getNodeAs<ValueDecl>(kType);
+ if (D) {
+ diag(D->getBeginLoc(), "type " + D->getType().getAsString() +
----------------
================
Comment at:
clang-tools-extra/clang-tidy/concurrency/AsyncBlockingCheck.cpp:299-300
+
+ const auto *CE = Result.Nodes.getNodeAs<CXXMemberCallExpr>(kMethod);
+ if (CE) {
+ if (Name) {
----------------
================
Comment at:
clang-tools-extra/clang-tidy/concurrency/AsyncBlockingCheck.cpp:302-304
+ diag(CE->getBeginLoc(), "method " + Name->getNameAsString() +
+ " may sleep and is not coroutine-safe")
+ << SourceRange(CE->getBeginLoc(), CE->getEndLoc());
----------------
Diagnostics support "%0" style formatting.
That formatting handles NamedDecls.
Stmt has a SourceRange that does the same job as (getBeginLoc(), getEndLoc()]
This can be applied in other places.
================
Comment at:
clang-tools-extra/clang-tidy/concurrency/AsyncBlockingCheck.cpp:308-309
+
+ const auto *E = Result.Nodes.getNodeAs<CallExpr>(kFunction);
+ if (E) {
+ if (Name) {
----------------
================
Comment at: clang-tools-extra/clang-tidy/concurrency/AsyncBlockingCheck.cpp:320
+ const auto *Lockfree = Result.Nodes.getNodeAs<VarDecl>(kLockfree);
+ if (Lockfree) {
+ const auto *EV = Lockfree->ensureEvaluatedStmt();
----------------
From the code, Lockfree can't be null if Atomic binds, maybe make this an assert
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/concurrency-async-blocking.cpp:450
+
+// TODO: remove CHECKT-MESSAGES
----------------
Whats this for?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93940/new/
https://reviews.llvm.org/D93940
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
