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.

Details

Summary

This commit is mostly for experimentation purposes so far.
If the new iteration order is more successful, it would replace the previous one.

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.