[PATCH] D51531: [analyzer][UninitializedObjectChecker] Uninit regions are only reported once

[Artem Dergachev via Phabricator via cfe-commits]

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

Mm, ok, i admit that i don't know what specific de-duplication do we want to have and what are the usability implications of it.

If we want de-duplication for the same region reported in different spots of the same path, we need a state trait.

If we want de-duplication for the same region globally across the whole analysis, we need a checker-wide set of regions, like what you're doing.

If we want de-duplication for the same region globally across the whole translation unit, we need a checker-wide set of AST nodes (eg., field chains) that describe regions in terms that outlive `ExprEngine`.

If we want de-duplication for different regions within the same constructor call site, we need a uniqueing location at the call site, like what you're doing.

If we want de-duplication for different regions within the same constructor itself, we don't need a uniqueing location.

I guess you should just do whatever you want here?

In https://reviews.llvm.org/D51531#1286110, @Szelethus wrote:

> Oh, and the reason why I think it would add a lot of complication: since only `ExprEngine` is avaible in the `checkEndAnalysis` callback, which, from what I can see, doesn't have a handly `isDead` method, so I'm not even sure how I'd implement it.


Mm, what do you need `isDead` for? Everything is dead at the end of the analysis, you need to clean up everything, otherwise you get use-after-free(?)



================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp:203
   const Stmt *CallSite = Context.getStackFrame()->getCallSite();
   if (CallSite)
     LocUsedForUniqueing = PathDiagnosticLocation::createBegin(
----------------
Hmm, does `LocUsedForUniqueing` remain uninitialized sometimes? I guess it's on the top frame. What does it do in this case? Why do we need a uniqueing location in other cases? I guess it only *increases* the amount of reports, right? I.e., constructors called from different call sites produce different reports(?)


https://reviews.llvm.org/D51531