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

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

steakhal added a comment.

I have concerns mostly about the cast visitor.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:192
+
+  const Stmt *S = N->getStmtForDiagnostics();
+  if (!S)
----------------
Discookie wrote:
> steakhal wrote:
> > Aren't you actually interested in N->getLocation().getAs<StmtPoint>().getStmt()?
> > 
> > The diag stmt can be fuzzy, but the PP is exact.
> As far as I can tell, getStmtForDiagnostics() does exactly that, but with a bit more edge case handling and a couple fallbacks.
If they do the same, and it does not depend on the mentioned fallbacks, I think we should use the Stmt of the programpoint to be explicit.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:194-201
   // Only interested in DerivedToBase implicit casts.
   // Explicit casts can have different CastKinds.
+  // FIXME: The checker currently matches all explicit casts,
+  // but only ones casting to a base class (or simular) should be matcherd.
   if (const auto *ImplCastE = dyn_cast<ImplicitCastExpr>(CastE)) {
     if (ImplCastE->getCastKind() != CK_DerivedToBase)
       return nullptr;
----------------
Have you considered `dyn_cast<CastExpr>()` to make it work for both implicit and explicit casts?
BTW `simular` -> `similar`


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:220
                              N->getLocationContext());
-  return std::make_shared<PathDiagnosticEventPiece>(Pos, OS.str(), true);
+  return std::make_shared<PathDiagnosticEventPiece>(Pos, OS.str(), /*addPosRange=*/true);
+}
----------------
We should assert that this visitor always adds a Note. In other words, that it must find the Stmt where the derived->base conversion happened. If ever that's not true, we have a bug.


================
Comment at: clang/test/Analysis/ArrayDelete.cpp:85-88
+    Derived *d3 = new DoubleDerived[10];
+    Base *b3 = d3; // expected-note{{Conversion from derived to base happened here}}
+    delete[] b3; // expected-warning{{Deleting an array of polymorphic objects is undefined}}
+    // expected-note at -1{{Deleting an array of polymorphic objects is undefined}}
----------------
Hmm, the static type of `d3` doesn't tell us much about how it refers to an object of type `DoubleDerived`.
To me, it would make sense to have multiple `Conversion from derived to base happened here`, even telling us what static type it converted to what other static type in the message.
And it should add a new visitor of the same kind tracking the castee.

```
Derived *d3 = new DoubleDerived[10]; // note: `DoubleDerived` -> `Derived` here
Base *b3 = d3; // note: `Derived` -> `Base` here
delete[] b3; // warn: Deleting `Derived` objects as `Base` objects.
```
WDYT @donat.nagy ?


================
Comment at: clang/test/Analysis/ArrayDelete.cpp:90-93
+    Base *b4 = new DoubleDerived[10];
+    Derived *d4 = reinterpret_cast<Derived*>(b4);
+    DoubleDerived *dd4 = reinterpret_cast<DoubleDerived*>(d4);
+    delete[] dd4; // no-warning
----------------
I think in such cases a `static_cast` should suffice; unless you intentionally wanted to test `reinterpret_cast` of course.


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

https://reviews.llvm.org/D158156