Page MenuHomePhabricator

[Analyzer][WebKit] UncountedLambdaCaptureChecker
Needs ReviewPublic

Authored by jkorous on Mon, Jun 29, 9:37 PM.

Details

Reviewers
NoQ

Diff Detail

Event Timeline

jkorous created this revision.Mon, Jun 29, 9:37 PM
NoQ added a comment.Mon, Jun 29, 10:41 PM

Normally this isn't a big deal but in case of this checker i'm veeery curious about the highlighted columns (not just lines) and source ranges. Especially for the implicit case: like, how obvious is it which specific captured variable is problematic? I don't see a better way to test that than making a few FileCheck tests with those ^~~~~~ thingies but i think it's worth at least a couple of such test cases.

You can also add extra source ranges manually with BugReport::addRange.

It's probably also worth it to include the name of the captured variable in the warning message.

I know what you mean - actionability of these warnings was my major concern too.

jkorous updated this revision to Diff 276869.Thu, Jul 9, 4:31 PM
  • There's actually no reason to not print the name of the variable in either case.
  • I added a test for each explicit/implicit capture and made tests more specific overall.
jkorous updated this revision to Diff 276877.Thu, Jul 9, 5:10 PM
jkorous edited the summary of this revision. (Show Details)
  • added documentation
NoQ added a comment.Thu, Jul 9, 7:16 PM

That looks much better, thanks! I've a few more tiny annoying nits.

clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
26

I recommend re-using this string because it's used by multiple checkers already.

We usually keep those in CommonBugCategories.h and CommonBugCategories.cpp.

81

I think it should be fairly easy to automatically figure out whether it's a raw pointer or a reference (?) I think we should get this polished in this patch if possible, because we're instantly turning the checker on by default.

clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
8–9

Oh my, i see what you did here!

I wonder if the second line actually tests the exact position of ^. Because by default {{xxx}} checks that xxx is a substring of the actual warning (in this case, notice). Might need regex matching to actually match the beginning and the end of the line.

15

So it points to the opening brace? I guess that's good enough with the name of the variable present in the warning text but there's room for experimentation - for instance, with some effort we could probably point to the first use of the variable; i'm not sure it's actually going to be much better but at least it'd point to the actual offending code.

34

Maybe let's put this comment inside so that nobody thought that it applies to the rest of the file :)

jkorous marked 2 inline comments as done.Thu, Jul 9, 11:30 PM

Oh no - nits are welcome as always!

I agree with you on all of account and will work on it in the morning.

clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
8–9

Oh my, now I see it too :(

More in the response below.

15

Oh no ... [facepalm]

You just proved your point that I am matching just a substring here - in reality (all the handful of cases I've seen) the caret point to the start of the identifier.

I'd be inclined to say that now that we're printing the name of the variable the snippet isn't critical anymore and we might not necessarily have tests for it. But to be honest I am actually not sure if the current behavior (the caret points to the first occurrence of the implicitly captured variable) is defined or not.

I'll think about it - would prefer to not use multi-line regex magic if possible. Maybe I could somehow use serialized diagnostics here?