[PATCH] D81315: [Draft] [Prototype] 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:46
+};
+} // end of anonymous namespace
+
----------------
You can merge the two anonymous namespaces, this and the one below.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:60
+private:
+  mutable std::unique_ptr<BugType> BT;
+  void reportBug(CheckerContext &C, const CallEvent &Call) const;
----------------
This is how we used to do it, but we did not update all the checks yet. Nowadays we can just initialize bugtypes immediately, see https://github.com/llvm/llvm-project/blob/master/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp#L169


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:65
+
+  // STL smart pointers which we are trying to model
+  const llvm::StringSet<> StdSmartPtrs = {
----------------
In LLVM we aim for full sentences as comments with a period at the end.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:119
+  ProgramStateRef State = C.getState();
+  const auto OC = dyn_cast<CXXMemberOperatorCall>(&Call);
+  if (!OC)
----------------
Here the const applies for the pointer, not the pointee. We usually do `const auto *OC` instead.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:134
+        reportBug(C, Call);
+      }
+    }
----------------
In case we did not report a bug, we could assume that the pointer is not-null and we could update the state accordingly. This is a common approach the analyzer takes. E.g. when we se a division by X, we either report a bug or add an assumption that X is non-zero. 


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:150
+      const CXXRecordDecl *RD = CtorDec->getParent();
+      if (isSmartPointer(RD)) {
+        State =
----------------
I wonder if you wanted to add `isSmartPointer` checks below as well. In that case you can hoist this check and early return for non-smart-pointers.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:195
+  // Clean up dead regions from the region map.
+  TrackedRegionMapTy TrackedRegions = State->get<TrackedRegionMap>();
+  for (auto E : TrackedRegions) {
----------------
You probably also want to clean up the `SymbolRegionMap`. It is ok to not do it right now, but we usually tend to add `FIXMEs` or `TODOs` early and aggressively to make sure we do not forget stuff. 


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:225
+  if (RecordDec->getDeclName().isIdentifier()) {
+    return StdSmartPtrs.count(RecordDec->getName().lower());
+  }
----------------
This looks good for now. But we sometimes cache the pointer to identifier info objects so after the initial lookup we can just do pointer comparison instead of more expensive operations. Also add a fixme to check for the `std` namespace.

Also, this method could even be promoted to a free functions making the list of SmarPtrs global. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81315