NoQ added a comment.
Thanks!!
I'm having second thoughts on re-using the existing builder. Most other
`runCheckers...()` methods are building their own. Given how confusing this
entire node builder business is, i believe we should not deviate from known
working patterns.
Please forgive me if i at some point decide to take over this patch and do this
myself. I have strong opinions about this entire NodeBuilder API and i'm pretty
much screaming internally when i try to understand why it's made the way it's
made, so there's a high chance i'll want to introduce some invasive sanity into
it on my own.
================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:619
getCheckerManager().runCheckersForEvalCall(DstEvaluated, *I, *Call,
*this,
- CallOpts);
+ CallOpts, Bldr);
}
----------------
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.
================
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}}
----------------
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?
================
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
}
----------------
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?
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
