[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtr.h:37
+
+class GetNoteVisitor : public BugReporterVisitor {
+private:
----------------
vsavchenko wrote:
> Sorry for picking on it again, but is it visiting "get notes"? Otherwise, it is just a placeholder name that doesn't tell the reader what this class is actually for.
> Also, is there a reason why it is declared in the header file and couldn't be a part of `SmartPtrChecker.cpp` instead?
Yes it is the line `const SVal *InnerPtr = State->get<TrackedRegionMap>(ThisRegion);` in the `VisitNode` function. This uses the `TrackedRegionMap` which is defined in `SmartPtrModelling.cpp` but the visitor must be added to the bug-report in `SmartPtrChecker.cpp`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:79
       {{"get"}, &SmartPtrModeling::handleGet}};
+  mutable llvm::ImmutableSet<const Expr *>::Factory StmtSetFactory;
 };
----------------
vsavchenko wrote:
> I think it's better to use `REGISTER_SET_FACTORY_WITH_PROGRAMSTATE` instead and keep factory as part of the state as well.
My bad, I should have put in the TODO.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:180
+  }
+  return std::shared_ptr<PathDiagnosticEventPiece>();
+}
----------------
vsavchenko wrote:
> Just wondering if `return {};` will be sufficient here.
Yup


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:632-633
+    auto EmptySet = StmtSetFactory.getEmptySet();
+    auto NewStmtSet = StmtSetFactory.add(EmptySet, CallExpr);
+    State = State->set<ExprsFromGet>(ThisRegion, NewStmtSet);
+  }
----------------
vsavchenko wrote:
> I generally don't like repeating code in both branches of the `if` statement.
> Here they share the following logic: add `CallExpr` and update the state.
> We can easily update the code to share the code:
> ```
> const auto *ExistingSet = State->get<ExprsFromGet>(ThisRegion);
> auto BaseSet = ExistingSet ? *ExistingSet : StmtSetFactory.getEmptySet();
> auto NewStmtSet = StmtSetFactory.add(BaseSet, CallExpr);
> State = State->set<ExprsFromGet>(ThisRegion, NewStmtSet);
> ```
Right, thanks :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97183