[PATCH] D81315: [analyzer][Draft] [Prototype] warning for default constructed unique pointer dereferences

[Valeriy Savchenko via Phabricator via cfe-commits]

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