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@-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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
