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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
