[PATCH] D158156: [analyzer] Add C++ array delete checker

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

I share the raised concerns. And I think we are aligned.

PS: sorry if any of my comments are dups, or outdated. I only had a little time last week, and things have progressed since then I noticed. I still decided to submit my possibly outdated and partial review comments. I hope you understand.
Its quite difficult to allocate time to do reviews from day to to day work. I unfortunately usually do this in my spare time, if I have.



================
Comment at: clang/docs/analyzer/checkers.rst:1787-1804
+.. _alpha-cplusplus-ArrayDelete:
+
+alpha.cplusplus.ArrayDelete (C++)
+""""""""""""""""""""""""""""""""""""""""""""""
+Reports destructions of arrays of polymorphic objects that are destructed as their base class.
+
+.. code-block:: cpp
----------------
I think you should probably mention `EXP51-CPP CERT rule` somehow here.


================
Comment at: clang/docs/analyzer/checkers.rst:1789-1791
+alpha.cplusplus.ArrayDelete (C++)
+""""""""""""""""""""""""""""""""""""""""""""""
+Reports destructions of arrays of polymorphic objects that are destructed as their base class.
----------------



================
Comment at: clang/docs/analyzer/checkers.rst:1793-1803
+.. code-block:: cpp
+
+ Base *create() {
+   Base *x = new Derived[10]; // note: conversion from derived to base
+                              //       happened here
+   return x;
+ }
----------------
Make sure in the example the `create` is related (e.g. called/used).
Also, refrain from using `sink` in the docs. It's usually used in the context of taint analysis.


================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:757-760
+def CXXArrayModeling: Checker<"CXXArrayModeling">,
+  HelpText<"The base of a few CXX array related checkers.">,
+  Documentation<NotDocumented>,
+  Hidden;
----------------



================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:50-51
+  class DeleteBugVisitor : public BugReporterVisitor {
+  public:
+    DeleteBugVisitor() : Satisfied(false) {}
+    void Profile(llvm::FoldingSetNodeID &ID) const override {
----------------



================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:61
+  private:
+    bool Satisfied;
+  };
----------------



================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:65-68
+  bool ArrayDeleteEnabled = false;
+  CheckerNameRef ArrayDeleteName;
+  bool NonVirtualDeleteEnabled = false;
+  CheckerNameRef NonVirtualDeleteName;
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158156