martong accepted this revision. martong added a comment. This revision is now accepted and ready to land.
For me it looks good now. The changes, however, seem to be unrelated to retain count checker. Do you think it would make sense to upstream this patch independently from the other 3 patches you have in your stack? That might require some changes in the test files though. ================ 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()); ---------------- vsavchenko wrote: > martong wrote: > > vsavchenko wrote: > > > 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. > > Yeah, I missed that about `FindUniqueBinding`. > > > > It's just side questions then: Could we handle multiple bindings by finding > > the 'last' binding and tracking that back? > > And in this example, what are the bindings for `c`? > Can you please elaborate on what you mean by 'last', like 'last used'? > As for `c`, you can see above that I'm looking for a node where 'c' is not > bound to that value. So, we start with the node where 'a', 'b', and 'c' all > bound to symbolic value... let's say 'x'. I go up the graph to the point > where 'c' is not bound to 'x' and there I look for unique binding for 'x'. > In that example, we won't find it because it has two: 'a' and 'b'. Thanks for the explanation! > Can you please elaborate on what you mean by 'last', like 'last used'? Yes, I meant the 'last used', in this case it should be `foo(b)`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101041/new/ https://reviews.llvm.org/D101041 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
