donat.nagy added a comment.
This checker is a straightforward, clean implementation of the SEI-CERT rule
EXP51-CPP, it'll be a good addition to Static Analyzer. I'm satisfied with the
implementation, although there might be others who have a more refined taste in
C++ coding style. The only issue that I found is the misleading name of the
"common ancestor" hidden checker that I mentioned in the inline comment.
The test suite nicely covers all the basic situations, but I'd be happy to see
some corner cases like virtual and non-virtual diamond inheritance or even a
degenerate example like
class Base { /*...*/ };
class Derived: public Base {
Base basesAsMember[10];
/*...*/
};
void f() {
Derived *d = new Derived(...);
delete[] (d->basesAsMember);
}
================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:757-760
+def CXXArrayModeling: Checker<"CXXArrayModeling">,
+ HelpText<"The base of a few CXX array related checkers.">,
+ Documentation<NotDocumented>,
+ Hidden;
----------------
The name and help text of this hidden dependency is very misleading: it's not
particularly connected to arrays (the connection between the two child checkers
is that they both handle the "delete" operator) and it's not a modeling checker
(which would provide function evaluation, constraints and transitions) but an
"empty shell" checker that does nothing by itself, but provides a singleton
checker object where the registration of the child checkers can enable the two
analysis modes.
Please rename this to `CXXDeleteHelper` or something similar.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:88
+ const auto *BaseClassRegion = MR->getAs<TypedValueRegion>();
+ const auto *DerivedClassRegion =
MR->getBaseRegion()->getAs<SymbolicRegion>();
+ if (!BaseClassRegion || !DerivedClassRegion)
----------------
This logic (which is inherited from the earlier checker) is a bit surprising
because it strips //all// Element, Field, BaseObject and DerivedObject layers
from the MemRegion object.
I think it works in practice because
(1) there is an `isDerivedFrom() check after it and
(2) it's rare to write (buggy) code like
```lang=c++
struct Inner { /*...*/ };
struct Outer { Inner inner; /*...*/ };
void f() {
Outer *o = new Outer(...);
delete(o->inner);
}
```
Perhaps this could be explained by a comment if you feel that it's warranted;
but I mostly wrote this as a note for myself and other reviewers.
Repository:
rG LLVM Github Monorepo
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
