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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked 5 inline comments as done.
Szelethus added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:59
 enum CallEventKind {
+  CE_CXXDeallocator,
   CE_Function,
----------------
xazax.hun wrote:
> Szelethus wrote:
> > I need to move this below `CE_Function`, as its not in the range of `CE_BEG_FUNCTION_CALLS` and `CE_END_FUNCTION_CALLS`.
> This is marked as done. If the conclusion is that you do not need to move it I think it would be worth to add a comment why :)
Marked done automatically, I'll address this before commiting :)


================
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
----------------
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.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:1083
+
+  const Expr *getArgExpr(unsigned Index = 0) const override {
+    return getOriginExpr()->getArgument();
----------------
xazax.hun wrote:
> Is the index unused on purpose? If yes, wouldn't this trigger a warning on build bots? Adding a maybe unused annotation and/or a comment would be welcome.
Unused argument warnings are disabled by the build system, but a comment wouldn't hurt. `CXXDeleteExpr`s can only have a single argument, but as earlier inlines suggests, we will need to mind custom deletes later on. 


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

https://reviews.llvm.org/D75430