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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Overall looks good to me some questions and nits inline.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:59
 enum CallEventKind {
+  CE_CXXDeallocator,
   CE_Function,
----------------
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 :)


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


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:1083
+
+  const Expr *getArgExpr(unsigned Index = 0) const override {
+    return getOriginExpr()->getArgument();
----------------
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.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp:95
   ExplodedNode *N = C.generateNonFatalErrorNode();
+  if (!N)
+    return;
----------------
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.


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

https://reviews.llvm.org/D75430