[PATCH] D86736: [analyzer][NFC] Don't bind values to ObjCForCollectionStmt, replace it with a GDM trait

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

I don't have anymore immediate concerns, but I will need more time to comb through the rest of the patch in more details... hopefully I can do that in the following days.



================
Comment at: clang/lib/StaticAnalyzer/Core/ProgramState.cpp:327
+using ObjCForLctxPair =
+    std::pair<const ObjCForCollectionStmt *, const LocationContext *>;
+
----------------
Szelethus wrote:
> martong wrote:
> > Why it is not enough to simply have ObjCForCollectionStmt* as a key?
> `Environment` is the data structure we use to bind **values** to **statements** (which would ideally be expressions only). The problem is that a statement on its own isn't enough to identify where we are in the analysis:
> 
> ```lang=c++
> void traverse(Graph &G) {
>  // SubG may need to be modeled in different stackframes, but has the same Stmt*
>  for (Graph &SubG : G->successors()) {
>     traverse(SubG);
>   }
> }
> ```
> 
> `LocationContext` can be thought of as a stack function calls (or anything with a distinct stack frame). Indeed, if two `(Stmt *, LocationContext *)` pairs are equal, we're rebinding the same expression (another loop iteration, goto), if only the statements are the same, we're not in the same function call (//stack frame//).
> 
> Of course `ObjCForCollectionStmt` can not have a value, that is the whole point of the patch, but we still need a `LocationContext` to correctly identify precisely which loop we're talking about.
Yeah, I did not think about stack frames, it makes perfect sense what you write, thanks for the explanation!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86736