vsavchenko marked 4 inline comments as done.
vsavchenko added inline comments.
================
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:
> 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)`.
That would be perfect actually, but not `foo(b)`, just `b`. Reporting `foo(b)`
won't give more information than the code itself already provides.
However, I don't know a simple way to get region `b` from `foo(b)` expression
(of course, we are talking about more general case than just getting the first
argument of `foo`).
================
Comment at: clang/test/Analysis/uninit-const.cpp:78
int t; //expected-note {{'t' declared without an initial value}}
- int &p = t; //expected-note {{'p' initialized here}}
- int &s = p; //expected-note {{'s' initialized here}}
- int &q = s; //expected-note {{'q' initialized here}}
+ int &p = t; //expected-note {{'p' initialized to the value of 't'}}
+ int &s = p; //expected-note {{'s' initialized to the value of 'p'}}
----------------
NoQ wrote:
> I suspect that this wording can be improved a lot. The note "`'p' initialized
> to the value of 't'`" sounds like an accurate description for `int p = t` but
> not so much for `int &p = t`. Can we detect this case and say something like
> "`'p' refers to 't'`" instead?
That's a good point, but it feels like a can miss some cases here and we need
to handle this situation separately.
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
