vsavchenko added a comment.
Great start!
I think you are on the right track, so maybe this code won't be thrown away at
all :-)
Try to work on tests and get familiar with `lit`.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:31
namespace {
-class SmartPtrModeling : public Checker<eval::Call> {
+struct RegionState {
+private:
----------------
xazax.hun wrote:
> I think `RegionState` is not very descriptive. I'd call it something like
> `RegionNullness`.
linter: LLVM coding standards require to use `class` keyword in situations like
this
(https://llvm.org/docs/CodingStandards.html#use-of-class-and-struct-keywords).
I would even say that `struct` is good for POD types.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:33
+private:
+ enum Kind { Null, NonNull, Unknown } K;
+ RegionState(Kind InK) : K(InK) {}
----------------
I think that it would be better to put declarations for `enum` and for a field
separately.
Additionally, I don't think that `K` is a very good name for a data member. It
should be evident from the name of the member what is it. Shot names like that
can be fine only for iterators or for certain `clang`-specific structures
because of existing traditions (like `SM` for `SourceManager` and `LO` for
`LanguageOptions`).
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:119
+ ProgramStateRef State = C.getState();
+ const auto OC = dyn_cast<CXXMemberOperatorCall>(&Call);
+ if (!OC)
----------------
xazax.hun wrote:
> Here the const applies for the pointer, not the pointee. We usually do `const
> auto *OC` instead.
As I said above, I think we should be really careful about abbreviated and
extremely short variable names. Longer names make it much easier to read the
code without paying a lot of attention to declarations.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:160-187
+ if (const auto IC = dyn_cast<CXXInstanceCall>(&Call)) {
+ const auto MethodDecl = dyn_cast_or_null<CXXMethodDecl>(IC->getDecl());
+ if (!MethodDecl)
+ return;
+ auto ArgsNum = IC->getNumArgs();
+
+ if (ArgsNum == 0 && isResetMethod(MethodDecl)) {
----------------
Considering the fact that more cases and more functions will get supported in
the future, I vote for merging common parts of these two blocks.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:230
+
+bool SmartPtrModeling::isResetMethod(const CXXMethodDecl *MethodDec) const {
+ if (!MethodDec)
----------------
I believe that methods (and data members related to them) can be `static`.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:233
+ return false;
+ if (MethodDec->getDeclName().isIdentifier()) {
+ return ResetMethods.count(MethodDec->getName().lower());
----------------
I'm not sure about it myself, but can `DeclName` be `isEmpty()`? If yes, it is
a potential null-pointer dereference. Again, I don't know it for a fact, but I
think it should be checked.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81315/new/
https://reviews.llvm.org/D81315
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
