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