Szelethus added a comment. In D75430#2028456 <https://reviews.llvm.org/D75430#2028456>, @NoQ wrote:
> Sorry i'm not very responsive these days >.< No worries, thanks! ^-^ ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:1052 +/// This is a call to "operator delete". +// FIXME: CXXDeleteExpr isn't present for custom delete operators, or even for +// some those that are in the standard library, like the no-throw or align_val ---------------- Szelethus wrote: > xazax.hun wrote: > > How important and hard it is to fix this? If you did not find the reason > > why those CXXDeleteExprs are missing you can do two things: > > 1. Look at how such AST is consumed by CodeGen > > 2. Ask around on the mailing list with a minimal example. > Fair point, but it surely is a thing to keep in mind because it could, or > more probably //will// cause surprises. A `TODO` or a `NOTE` would be more > appropriate. In any case, I'd prefer to address these in a followup patch. Actually, see my other inline. I do believe that this should be the one class to solve all deletes, or at the very least, standard-specified deletes. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp:95 ExplodedNode *N = C.generateNonFatalErrorNode(); + if (!N) + return; ---------------- NoQ wrote: > xazax.hun wrote: > > I think this change might be a better fit for a separate commit. I think > > you don't even need to have such a small change reviewed. You could just > > commit it as is. Just do not forget to describe that these cases are really > > hard to have a test for but this is the idiomatic way of doing this in > > checkers. > I'm also curious why did this suddenly become necessary. This is obviously > the right thing to do but why the change? It might indicate that we > accidentally started incorrectly agglutinating nodes that were previously > considered different. Like, we might have messed up our program points > somewhere in this patch. The creduced program that causes this checker to crash (without the early return): ```lang=cpp struct a {}; struct b : a {}; void c() { a *d = new b; delete d; } ``` Turns out, as I said in an another inline, I accidentally run `PreStmt` on delete expressions twice. In any case, I decided to add this early return and a test case in this patch, because I think strongly related, and as seen here, tests the added functionality. ================ Comment at: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:1652 + ExplodedNodeSet PostVisit; + getCheckerManager().runCheckersForPreStmt(PostVisit, PreVisit, S, *this); ---------------- Oh wow. I accidentally ran checkers on `PreStmt` here as well. Thats why I needed the new early return to not crash. ================ Comment at: clang/test/Analysis/cxx-dynamic-memory-analysis-order.cpp:31-32 - // FIXME: All calls to operator new should be CXXAllocatorCall. - // FIXME: PostStmt<CXXDeleteExpr> should be present. + // FIXME: All calls to operator new should be CXXAllocatorCall, and calls to + // operator delete should be CXXDeallocatorCall. { ---------------- NoQ wrote: > That's fairly unobvious to me. Isn't the whole point of > `CXXAllocatorCall`/`CXXDeallocatorCall` to give access to specific aspects of > new/delete-expressions that aren't available in manual invocations with > function syntax? On the other hand, i agree that it's nice to be able to cast > the call event to `CXXAllocatorCall`/`CXXDeallocatorCall` and be done with > allocators/deallocators. Yup, in my mind, this isn't `CXXNewCall`, but rather `CXXAllocatorCall` (same for delete), I would expect this to handle it all (maybe change the interfaces so that `getOriginExpr` returns with `Expr`, and add a `getOriginAsCXXNewExpr` method). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75430/new/ https://reviews.llvm.org/D75430 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits