[PATCH] D71152: [analyzer] Keep track of escaped locals.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Looks great, thanks! I'd love to see what effect it immediately causes and whether there will be any true or false negatives because of this change.

Also i-i-it's not that i insist or something, but somebody will have to add it to the exploded graph dumps :)



================
Comment at: clang/lib/StaticAnalyzer/Core/ProgramState.cpp:48-61
+namespace {
+struct EscapedLocals{};
+} // namespace
+
+template <>
+struct ProgramStateTrait<EscapedLocals> :
+  public ProgramStatePartialTrait<llvm::ImmutableSet<const MemRegion *>> {
----------------
xazax.hun wrote:
> xazax.hun wrote:
> > NoQ wrote:
> > > Wait, you are preventing direct access anyway by putting this stuff into the .cpp file.
> > > 
> > > In this case i think you can safely use the `REGISTER_...` macros.
> > Yeah, initially I wanted to hide this trait but at this point I have no idea how to do that. If I get the layering right, ExprEngine is using ProgramState, and ProgramState should not reference ExprEngine. So if I want to use a trait in both it should be in ProgramState. The reason why I also need to touch ProgramState, because the list of invalidated regions are readily available there. Probably it is possible to somehow get that list in ExprEngine, but I think that would be way more complicated than the current solution.
> Well, I realized that ProgramState can access SubEngine, so as long as we extend the abstract interface there we could move the trait to ExprEngine. What do you prefer? 
I very slightly prefer to have it in `ExprEngine` because it's consistent with other existing traits: defined in the place in which they're maintained, don't show up in the overall definition of the `ProgramState`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71152/new/

https://reviews.llvm.org/D71152