danix800 wrote:

> > Thanks for the commit, I'm satisfied with it :)
> > I actually like that these two related changes (the checker change and the 
> > constraint manager improvement) are handled together in a single commit -- 
> > this way somebody who browses the commit log can directly see the "other 
> > half" of the change (without following cumbersome links through "this 
> > commit is mentioned in the commit message of that one" or opening this 
> > github review). Also, the checker change is so trivial (+2 lines) that the 
> > full combined commit is still small enough to be easily understood.
> 
> I agree.
> 
> The patch looks good to me. I made some recommendations for improving the 
> tests.
> 
> I have one question. In the StdLibraryFn checker you added a test for 
> `isSink()`. I thought that the crash was that some `assume(..., true|false)` 
> returned a null State, that we dereferenced. Where was that assume call and 
> how did we unconditionally dereference it?

It's not a null state being dereferenced, but an assertion failure crash. The 
checker operates on some impossible states
and try to add one of the states back to the graph, which is rejected 
(assertion failed).

https://github.com/llvm/llvm-project/pull/115579
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to