donat.nagy added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:47
+protected:
+ class PtrCastVisitor : public BugReporterVisitor {
public:
----------------
I like that yo switched to a more descriptive class name :)
================
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}}
----------------
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.
================
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
----------------
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
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
