Page MenuHomePhabricator

[analyzer] Experiment with an iteration order only based on location, and not using the stack frame
ClosedPublic

Authored by george.karpenkov on Oct 9 2018, 6:42 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

I remember when I joined the project how my team used to analyze projects with every iteration order to which is the best, so I think this patch will be appreciated.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/WorkList.h
61 ↗(On Diff #168933)

Completely unrelated to this revision, but this class needs some doc desperately IMO :/

clang/lib/StaticAnalyzer/Core/WorkList.cpp
258 ↗(On Diff #168933)

CFGBlock::getBlockID returns with an unsigned int, did you mean this to be signed? I don't mean to be a living and breathing clang-tidy, but you explicitly mentioned using signed integer in the comment below.

272 ↗(On Diff #168933)

I believe some bots like to complain if you don't make operator() const.

299 ↗(On Diff #168933)

How about queue.push({U, {-NumVisited, ++Counter}});?

NoQ added a comment.Oct 10 2018, 3:35 PM

Let's see how it goes!

That's a lot of code duplication, i guess it might make sense to refactor later.

clang/lib/StaticAnalyzer/Core/WorkList.cpp
258 ↗(On Diff #168933)

No, it doesn't need to be int; the comment below is about the other int.

NoQ accepted this revision.Oct 10 2018, 3:36 PM
This revision is now accepted and ready to land.Oct 10 2018, 3:36 PM
This revision was automatically updated to reflect the committed changes.
NoQ added a comment.Oct 11 2018, 5:43 PM

A few post-commit thoughts.

cfe/trunk/lib/StaticAnalyzer/Core/WorkList.cpp
284

I usually imagine queue as a horizontal thing, so it's unclear to me whether "top" is "left" or "right".

297

Does this mean that Block 1 in foo() and Block 1 in bar() will always have the same NumReached value?

cfe/trunk/lib/StaticAnalyzer/Core/WorkList.cpp
297

That's a great point. Pushing an update.