vrnithinkumar marked 5 inline comments as done.
vrnithinkumar added a comment.
================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:619
getCheckerManager().runCheckersForEvalCall(DstEvaluated, *I, *Call,
*this,
- CallOpts);
+ CallOpts, Bldr);
}
----------------
NoQ wrote:
> vrnithinkumar wrote:
> > NoQ wrote:
> > > We should probably delete the copy-constructor for node builders. I've no
> > > idea what it's supposed to do anyway and the whole problem that we're
> > > having here is due to there being //too many// of them already.
> > So we should disable the copying of `NodeBuilder` and create a heap
> > allocated `NodeBuilder` and use pointer to pass around functions?
> No-no, keep it on the stack and don't pass it around *at all*.
Sorry, I am still little confused.
Do we have to make the fix without passing around the `NodeBuilder`?
And delete copy-constructor for `NodeBuilder` in this patch?
================
Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:39
void derefAfterRelease() {
- std::unique_ptr<A> P(new A());
+ std::unique_ptr<A> P(new A()); // expected-note {{Smart pointer 'P' is
constructed}}
P.release(); // expected-note {{Smart pointer 'P' is released and set to
null}}
----------------
NoQ wrote:
> vrnithinkumar wrote:
> > NoQ wrote:
> > > Ok, these notes shouldn't be there; a note on `.release()` is sufficient
> > > to understand the warning and it looks like that's one more place where
> > > we should mark the region as uninteresting.
> > >
> > > Can you try to debug why did they suddenly show up?
> > I checked the exploded graph for this test case.
> > Before the bug fix, there exists a path where the no Note Tag is added to
> > the corresponding `CXXConstructExpr`. After the fix removed this branching
> > theres always a Note Tag on Ctr. {F12591752}
> >
> > Since the note on .release() is sufficient to understand the warning and I
> > agree we should mark this region as uninteresting.
> Ok, fair enough! Let's add a FIXME.
Added the FIXME
================
Comment at: clang/test/Analysis/smart-ptr.cpp:143
pass_smart_ptr_by_const_rvalue_ref(std::move(P));
- P->foo(); // expected-warning {{Dereference of null smart pointer 'P'
[alpha.cplusplus.SmartPtr]}}
+ P->foo(); // no-warning
}
----------------
NoQ wrote:
> That's indeed the intended consequence of your patch: we're no longer
> exploring a bogus execution path that's parallel to the previous null
> dereference on line 133, therefore we're no longer emitting this warning but
> instead correctly interrupting the entire analysis after we've found that
> other null dereference.
>
> That said, we should preserve the test so that it was still testing whatever
> it was testing previously. Can you split up this function into smaller
> functions so that each test was running independently?
Separated the function into smaller functions.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D85796/new/
https://reviews.llvm.org/D85796
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
