Page MenuHomePhabricator

[analyzer] AST-matching checker to detect global central dispatch performance anti-pattern
ClosedPublic

Authored by george.karpenkov on Mar 2 2018, 4:55 PM.

Diff Detail

Repository
rC Clang

Event Timeline

Hello George. I don't see any problems; just a comment inline.

lib/StaticAnalyzer/Checkers/GCDAsyncSemaphorChecker.cpp
71 ↗(On Diff #136885)

hasDescendant() will fire only once even if there are multiple usages of this pattern in same function. If it can be a common case, it is better to use forEachDescendant() instead; if not, could you add a comment describing your decision?

george.karpenkov retitled this revision from [analyzer] [WIP] AST-matching checker to detect global central dispatch performance anti-pattern to [analyzer] AST-matching checker to detect global central dispatch performance anti-pattern.
george.karpenkov edited the summary of this revision. (Show Details)Mar 5 2018, 12:14 PM
NoQ added inline comments.Mar 5 2018, 12:27 PM
lib/StaticAnalyzer/Checkers/GCDAsyncSemaphorChecker.cpp
71 ↗(On Diff #136885)

Because we need a descendant for which all three hold, i guess it'd take a dual-layer construct (forEachDescendant(compoundStmt(hasDescendant, hasDescendant, hasDescendant))) and then somehow sort them to pick the smallest match if nested compound statements are present (if we need, but i guess it's not necessary because we won't highlight the whole statement, and unique-ing will do the rest for us).

lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp
66–67

*crosses fingers for that to compile on all buildbots*

86

To me this sounds like a named constant that stores the name of another named constant.

NoQ added inline comments.Mar 5 2018, 12:30 PM
test/gcdasyncsemaphorechecker_test.m
14–16

Looks like an accidental leftover.

lib/StaticAnalyzer/Checkers/GCDAsyncSemaphorChecker.cpp
71 ↗(On Diff #136885)

I guess it depends on whether we want to add path notes for signaling and creating the semaphore. If not, just changing all hasDescendant to forEachDescendant somehow magically works.

lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp
66–67

Seems standard C++11?...

86

Do you propose to rename it? To what?

george.karpenkov marked an inline comment as done.
george.karpenkov marked an inline comment as done.
NoQ added inline comments.Mar 5 2018, 1:19 PM
lib/StaticAnalyzer/Checkers/GCDAsyncSemaphorChecker.cpp
71 ↗(On Diff #136885)

Heh. I guess you could add a test with two wait calls on the same semaphore and mark it as a FIXME.

lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp
86

Just use the string everywhere?

lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp
86

But the current usage protects against typos in multiple usages.

NoQ accepted this revision.Mar 5 2018, 1:55 PM

Yay lgtm now. Let's see how it evolves :)

lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp
86

Hmm okay.

This revision is now accepted and ready to land.Mar 5 2018, 1:55 PM
This revision was automatically updated to reflect the committed changes.

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

include/clang/StaticAnalyzer/Checkers/Checkers.td
615

This should have a more general name so that we can add related checks to it in the future. I suggest "GCDAntipattern".

616

We don't use "checker" as a term in user-facing text. I suggest "Check for performance anti-patterns when using Grand Central Dispatch".

lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp
12

Nit: missing "e" at end of "semaphor".

23

Nit: same here "semaphores"

92

It would be good to look at both the method/function name and the class name name for "test", "Test", "mock", and "Mock".

144

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"