[PATCH] D44059: [analyzer] AST-matching checker to detect global central dispatch performance anti-pattern

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin added a comment.

This looks good. Some minor post-commit review inline.



================
Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:615
+
+def GCDAsyncSemaphoreChecker : Checker<"GCDAsyncSemaphore">,
+  HelpText<"Checker for performance anti-pattern when using semaphors from async API">,
----------------
This should have a more general name so that we can add related checks to it in the future. I suggest "GCDAntipattern".


================
Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:616
+def GCDAsyncSemaphoreChecker : Checker<"GCDAsyncSemaphore">,
+  HelpText<"Checker for performance anti-pattern when using semaphors from async API">,
+  DescFile<"GCDAsyncSemaphoreChecker.cpp">;
----------------
We don't use "checker" as a term in user-facing text. I suggest "Check for performance anti-patterns when using Grand Central Dispatch".


================
Comment at: lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp:12
+// antipattern when synchronous API is emulated from asynchronous callbacks
+// using a semaphor:
+//
----------------
Nit: missing "e" at end of "semaphor".


================
Comment at: lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp:23
+// Such code is a common performance problem, due to inability of GCD to
+// properly handle QoS when a combination of queues and semaphors is used.
+// Good code would either use asynchronous API (when available), or perform
----------------
Nit: same here "semaphores"


================
Comment at: lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp:92
+  if (const auto* ND = dyn_cast<NamedDecl>(D))
+    if (ND->getName().startswith("test"))
+      return;
----------------
It would be good to look at both the method/function name and the class name name for "test", "Test", "mock", and "Mock".


================
Comment at: lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp:144
+      /*Category=*/"Performance",
+      "Possible semaphore performance anti-pattern: wait on a semaphore "
+      "signalled to in a callback",
----------------
We should tell the user more information about why they should believe this is bad.

I suggest "Waiting on a semaphore with Grand Central Dispatch creates useless threads and is subject to priority inversion"


Repository:
  rC Clang

https://reviews.llvm.org/D44059