Page MenuHomePhabricator

[analyzer] Use a custom program point for the check::NewAllocator callback.

Authored by NoQ on Jan 5 2018, 5:49 PM.



This addresses a TODO from D41406. I re-used PostImplicitCall program point when calling the new callback, but it indeed causes problems: because implicit call has no associated statement, MallocChecker was unable to put visitor diagnostics over these nodes. Create a new class of ProgramPoint, namely PostAllocatorCall, which is a sub-class of StmtPoint, and use that when calling the callback.

This patch might cause slight conflicts with D41150.

Diff Detail

rC Clang

Event Timeline

NoQ created this revision.Jan 5 2018, 5:49 PM
NoQ added inline comments.Jan 5 2018, 5:50 PM

dcoughlin accepted this revision.Jan 8 2018, 6:22 PM
dcoughlin added inline comments.

Can we get rid of the TODO now?

This revision is now accepted and ready to land.Jan 8 2018, 6:22 PM
xazax.hun accepted this revision.Jan 9 2018, 5:49 AM



Maybe = default is getting more canonical within LLVM? But that would not match the rest of the file, so I am fine with not touching this.


I think this is fine for now, but I wonder if in the future it would make more sense to have a getName or similar method for ProgramPoints.

NoQ updated this revision to Diff 129215.Jan 9 2018, 7:41 PM
NoQ marked an inline comment as done.

Fix the comment.

NoQ added inline comments.Jan 9 2018, 7:43 PM

Yeah, i guess i'd do a separate cleanup for these things.

a.sidorin accepted this revision.Jan 11 2018, 5:15 AM
a.sidorin added inline comments.

Not even a minor concern for this patch, but I think that placing //RUN and //CHECK after the code being tested could save us from massive changes of line numbers.

NoQ added inline comments.Jan 11 2018, 9:29 AM

Hmm, not sure if i understand, you mean before the code? (it would save us from line number changes in plists, but it'd make the tests harder to read because you'd have to scroll all the way down through the plist to find the actual code).

a.sidorin added inline comments.Jan 11 2018, 9:36 AM

I mean placing RUNs after the program code (but before // CHECK. Anyway, moving RUNs below will cause... line changes so it is not an important issue.

NoQ added inline comments.Jan 11 2018, 10:52 AM

Hmm, yeah, right, maybe. So there would be no line number issues until we stick a test in between two other tests or modify existing tests. But i'm still too used to having RUNs above everything, i guess, to just quickly figure out what does the test do. Also it's not hard to regenerate line numbers (even if it looks scary).

This revision was automatically updated to reflect the committed changes.