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

[Discookie via Phabricator via cfe-commits]

Discookie added inline comments.


================
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;
+ }
----------------
donat.nagy wrote:
> steakhal wrote:
> > Discookie wrote:
> > > steakhal wrote:
> > > > 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.
> > > Changed the example - should I change the DeleteWithNonVirtualDtor example as well? That has the same issues as you said here.
> > I have no opinion. You could.
> I think you should definitely update it, for the sake of consistency and just improving things whenever we can. This commit already touches the code of that checker, there is no point in leaving bad documentation behind us...
Updated.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:126
+
+  if (!DerivedClass->isDerivedFrom(BaseClass))
+    return;
----------------
steakhal wrote:
> Is this transitive?
> 
> BTW inheritance can only be expressed if the class is a definition, right?
> Thus passing this should imply has definition.
It's transitive indeed.

I thought there were some presumptions about hasDefinition() in isDerivedFrom(), but apparently not, and it didn't cause any crashes either. Removed the redundant checks.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:192
+
+  const Stmt *S = N->getStmtForDiagnostics();
+  if (!S)
----------------
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.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:196
+
+  const auto *CastE = dyn_cast<CastExpr>(S);
+  if (!CastE)
----------------
steakhal wrote:
> If you dyncast to ImplicitCastExpr, couldn't you have done it here?
I'm interested in all cast expressions, not just implicit ones. There can also be explicit upcasts as well.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:202
+  // Explicit casts can have different CastKinds.
+  if (const auto *ImplCastE = dyn_cast<ImplicitCastExpr>(CastE)) {
+    if (ImplCastE->getCastKind() != CK_DerivedToBase)
----------------
steakhal wrote:
> How do you know that that this castexpr corresponds to the region for which you report the bug? To mez this might be some unrelated castexpr.
> I was expecting the offending memregion being passed to the visitor, that it can check against.
We know it's the one we are looking for, because the checker marked it interesting earlier. Not sure how memregion equality check would differ from taking isInteresting(), but it can be changed to that if needed.


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

https://reviews.llvm.org/D158156