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