[PATCH] D101041: [analyzer] Find better description for tracked symbolic values

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1529
+      //     b = a;
+      //     c = foo(b);
+      //
----------------
martong wrote:
> I'd rather use `identity` here (and at line 1509) instead of `foo`, I think that could make this explanation easier to follow.
Good idea!


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1532
+      // Telling the user that the value of 'a' is assigned to 'c', while
+      // correct, can be confusing.
+      StoreManager::FindUniqueBinding FB(V.getAsLocSymbol());
----------------
martong wrote:
> So here, we have two or three bindings attached to `c`? `foo(b)` and `a` (and `b` as well) ?
> What is the guarantee that `FindUniqueBinding` will return with the correct one `foo(b)`? Seems like we iterate through an ImmutableMap (AVL tree) with `MemRegion*` keys. And the order of the iteration depends on pointer values. What I want to express, is that I don't see how do we find the correct binding, seems like we just find one, which might be the one we look for if we are lucky.
> 
> Perhaps we could have a lit test for this example?
Good idea, I should a test for that!
`FindUniqueBinding` does not only have a region, it also carries a flag checking if there are more than one binding.  `operator bool` then checks for both, thus guaranteeing that we won't choose random binding out of more than 1 - we won't choose at all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101041