[PATCH] D81315: [analyzer] Warning for default constructed unique pointer dereferences

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:35
   bool isNullAfterMoveMethod(const CallEvent &Call) const;
+  BugType NullDereferenceBugType{this, "Null-smartPtr-deref",
+                                 "C++ smart pointer"};
----------------
Nit: We do not use hypens/dashes in diagnostic names.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:53
+  // Set of STL smart pointer class which we are trying to model.
+  const llvm::StringSet<> StdSmartPtrs = {
+      "shared_ptr",
----------------
It might be just a personal preference but I tend to put these at the top of the file for easier discoverability. Feel free to ignore this comment unless other reviewers have the same preference as me.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:81
                                 CheckerContext &C) const {
-  if (!isNullAfterMoveMethod(Call))
+  if (isNullAfterMoveMethod(Call)) {
+    ProgramStateRef State = C.getState();
----------------
Don't we want this to be also protected by the `isStdSmartPointerClass` check?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:115
+  const auto *MethodDecl = dyn_cast<CXXMethodDecl>(Call.getDecl());
+  if (!MethodDecl || !isStdSmartPointerClass(MethodDecl->getParent()))
+    return;
----------------
I wonder if making `isStdSmartPointerClass` operate on `CallEvent` would simllify the call sites of this function.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:137
+    if (IsRegDead) {
+      State = State->remove<TrackedRegionMap>(Region);
+    }
----------------
In LLVM we often omit braces for small single statement branches. Also, Artem mentioned that we could interact with the malloc checker. Maybe it is worth considering to notify the malloc checker to mark the appropriate region as deallocated? This would help find double release errors, avoid spurious leak errors and so on. 


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:155
+
+  if (MethodDecl && MethodDecl->isOverloadedOperator()) {
+    OverloadedOperatorKind OOK = MethodDecl->getOverloadedOperator();
----------------
Early returns can decrease the indentation. 


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:213
+  if (NumArgs == 0) {
+    auto NullSVal = C.getSValBuilder().makeNull();
+    State = State->set<TrackedRegionMap>(ThisValRegion, NullSVal);
----------------
I wonder if we should use `makeNullWithType`. @NoQ what do you think? I am always confused when should we prefer one over the other.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81315/new/

https://reviews.llvm.org/D81315