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

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtr.h:37
+
+class GetNoteVisitor : public BugReporterVisitor {
+private:
----------------
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?


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


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:157
+
+PathDiagnosticPieceRef GetNoteVisitor::bugReportOnGet(const ExplodedNode *Node,
+                                                      BugReporterContext &BRC,
----------------
Functions are actions, it is better to express actions with verbs.
Additionally, I don't think that it really reflects what it does without the verb either.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:163-165
+    if (Method->getName() == "get" &&
+        Record->getDeclContext()->isStdNamespace() &&
+        Record->getName() == "unique_ptr") {
----------------
This type of predicates shouldn't be scattered throughout the code here and there.  It should be definitely unified and put into a function that is shared with the checker and other parts of the model.
One simple example here - what if we need to support other types of smart pointers? Should we go around all over the code looking for the places like this or fix it in one place?
It also naturally creates a question "What about `shared_ptr`"?


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


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:188-191
+  if (!E)
+    return;
+  if (E->getCastKind() != CastKind::CK_PointerToBoolean)
+    return;
----------------
I think it's better to unite these two into `if (!E || E->getCastKind()...)`


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:194-196
+  const Environment &Env = State->getEnvironment();
+  EnvironmentEntry Entry(Sub, Node->getLocationContext());
+  const SVal ExprVal = Env.getSVal(Entry, Bldr);
----------------
I guess it escaped during code refactoring:
`SVal ExprVal = State->getSVal(Sub, Node->getLocationContext());`


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:212-215
+  if (!BO)
+    return nullptr;
+  if (BO->getOpcode() != BO_Assign)
+    return nullptr;
----------------
Similar note here: `if (!BO || BO->getOpcode()...)`


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:218-221
+  if (!DeclRef)
+    return nullptr;
+  if (!PtrsTrackedAndConstrained.contains(DeclRef->getDecl()))
+    return nullptr;
----------------
And here


================
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);
+  }
----------------
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);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97183