Discookie marked 12 inline comments as done.
Discookie added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:192
+
+ const Stmt *S = N->getStmtForDiagnostics();
+ if (!S)
----------------
steakhal wrote:
> 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.
I'd rather not, since this pattern isn't used anywhere else, while
getStmtForDiagnostics is widely used across the codebase.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:217
+ // Stop traversal on this path.
+ Satisfied = true;
+
----------------
steakhal wrote:
> There are so many early returns going on. I feel, we could miss the program
> point where it should have been marked satisfied. After this point, the
> visitor will never or should never emit a note.
Since we're emitting notes for all casts, this isn't needed anymore.
================
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;
----------------
steakhal wrote:
> Have you considered `dyn_cast<CastExpr>()` to make it work for both implicit
> and explicit casts?
> BTW `simular` -> `similar`
I did exactly that a little earlier :D but I reworked the logic to filter based
on the availability of types instead of the CastType.
================
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}}
----------------
donat.nagy wrote:
> steakhal wrote:
> > donat.nagy wrote:
> > > steakhal wrote:
> > > > 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 ?
> > > I agree that it would be good to be good to mention the class names in
> > > the message.
> > Do you also agree that we should have all steps where such a conversion
> > happened?
> > Notice the 2 `note:` markers in my example. @donat.nagy
> It would be a nice addition if it wouldn't seriously complicate the
> implementation.
>
> If we want to report multiple/all conversions, then we would need to create
> messages for back-and-forth conversions (e.g. allocating Derived, converting
> it to Base, back to Derived, back to Base, then deleting it illegally).
Added, as requested! While casts of reference types are not supported, it
already makes the flow of classes much clearer.
================
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
----------------
donat.nagy wrote:
> steakhal wrote:
> > I think in such cases a `static_cast` should suffice; unless you
> > intentionally wanted to test `reinterpret_cast` of course.
> Note that the effects of `reinterpret_cast` and `static_cast` are different
> [1]: `static_cast<Derived*>(BasePtr)` adjusts the pointer value to ensure
> that it points to the derived object whose Base ancestor object would be
> located at BasePtr, while a `reinterpret_cast` keeps the raw pointer value
> (which is complete nonsense from an object-oriented point-of-view, but could
> be relevant in some low-level hacks).
>
> I'd guess that this part is directly testing the strange behavior of
> `reinterpret_cast`; but it'd be good to add a comment that clarifies this.
>
> [1]:
> https://stackoverflow.com/questions/9138730/why-is-it-important-to-use-static-cast-instead-of-reinterpret-cast-here
Sadly there isn't any deeper meaning to `reinterpret_cast` here, just a mistake
on my part. Fixed and changed to `static_cast`.
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
