[PATCH] D75430: [analyzer][NFC] Introduce CXXDeallocatorCall, deploy it in MallocChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I love where this is going.

> the reason behind this is that neither does `preStmt<CXXDeleteExpr>` have a `postStmt<CXXDeleteExpr>` pair. But I have no clue why that is :^)

I'm pretty sure it's forgotten :/



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:67
   CE_CXXAllocator,
+  CE_CXXDeallocator,
   CE_BEG_FUNCTION_CALLS = CE_Function,
----------------
After this you probably received compiler warnings saying "case isn't covered in switch". You'll need to clean them up.

Another thing to do would be to update `CallEventManager`'s `getCall()` and `getCaller()` methods that'd allow the users to construct the new `CallEvent` easily without thinking about what specific kind of call event it is.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:958
+
+  unsigned getNumArgs() const override { return getDecl()->getNumParams(); }
+
----------------
Charusso wrote:
> `return getOriginExpr()->getNumArgs()`
This wouldn't compile. `CXXDeleteExpr` is not a `CallExpr`.

It sounds like there are currently [[ http://www.cplusplus.com/reference/new/operator%20delete/ | five different `operator delete`s ]]:
{F11474783}

And, unlike `operator new`, there's no option to provide custom "placement" arguments.

So i think the logic in this patch is correct but we should do some custom logic for all 5 cases in the `getArgExpr` method, where argument expressions for the extra arguments will have to be conjured out of thin air (or we might as well return null).


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:853
+      CDE, Pred->getState(), Pred->getLocationContext());
+  getCheckerManager().runCheckersForPreCall(Dst, Pred, *Call, *this);
 }
----------------
Yes, we should absolutely have `PostCall` here as well, even if nothing happens in between.

And once it's there, the next major step would be to invoke `ExprEngine::evalCall()` instead if the operator was overloaded, so that we could //inline// the deallocator (or evaluate it conservatively, which might also be of value as it would invalidate the necessary globals).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75430/new/

https://reviews.llvm.org/D75430