[PATCH] D93940: [clang-tidy] Add a check for blocking types and functions.

[Nathan James via Phabricator via cfe-commits]

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