vsavchenko added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtr.h:36
+
+class FindWhereConstrained : public BugReporterVisitor {
+private:
----------------
nit: class name should be a noun (functions and methods are verbs)
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:84
REGISTER_MAP_WITH_PROGRAMSTATE(TrackedRegionMap, const MemRegion *, SVal)
+// REGISTER_SET_WITH_PROGRAMSTATE(ExprsFromGet, const Stmt *)
+REGISTER_MAP_WITH_PROGRAMSTATE(ExprsFromGet, const MemRegion *,
----------------
Probably forgotten
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:136-139
+ if (StmtsCovered.contains(S)) {
+ return nullptr;
+ }
+ StmtsCovered.insert(S);
----------------
We probably should have a checker in clang-tidy (maybe we already do), for
situations like this.
C++ has a long-lasting tradition that set's/map's `insert` return a pair:
iterator + bool. The second part tells the user if the `insert` operation
actually added the new element or it was already there.
This allows us to do 1 search instead of two.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:141-163
+ auto bugReportOnGet = [&](const Expr *E) -> auto {
+ if (const auto *MCE = llvm::dyn_cast<CXXMemberCallExpr>(E)) {
+ const auto *Method = MCE->getMethodDecl();
+ const auto *Record = MCE->getRecordDecl();
+ if (Method->getName() == "get" &&
+ Record->getDeclContext()->isStdNamespace() &&
+ Record->getName() == "unique_ptr") {
----------------
Is there any reason it's not a method of `FindWhereConstrained`?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:165
+
+ // If statement on raw pointer
+ if (const auto *E = llvm::dyn_cast<ImplicitCastExpr>(S)) {
----------------
After that you have 3 distinct cases to handle. It's a good opportunity for
extracting them into separate functions.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:179-180
+ // P.get()
+ if (const auto *InnerCastExpr =
+ llvm::dyn_cast<ImplicitCastExpr>(Sub)) {
+ Sub = InnerCastExpr->getSubExpr();
----------------
I think it's better to `IgnoreParensAndCasts` instead of manual traversal.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:216
+ // If b is a get() expression, then we can return a note
+ auto report = bugReportOnGet(RHS);
+ if (report) {
----------------
Variables are capitalized.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:217
+ auto report = bugReportOnGet(RHS);
+ if (report) {
+ return report;
----------------
It is a widespread pattern in LLVM to declare such variables directly in `if`
statements:
```
if (auto Report = bugReportOnGet(RHS))
return Report;
```
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:227
+ if (const auto *DS = llvm::dyn_cast<DeclStmt>(S)) {
+ const Decl *D = DS->getSingleDecl();
+ if (const auto *VD = llvm::dyn_cast<VarDecl>(D)) {
----------------
So, situations like `int *a = nullptr, *b = smart.get();` are not supported?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:229
+ if (const auto *VD = llvm::dyn_cast<VarDecl>(D)) {
+ for (const auto *I : PtrsTrackedAndConstrained) {
+ if (I->getName() == VD->getName()) {
----------------
`llvm::find_if`
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:238-245
+ }
+ }
+ }
+ }
+ }
+ }
+ }
----------------
This level of nestedness is frowned upon. It is a good tell that the function
should be refactored.
The following code:
```
if (cond1) {
. . .
if (cond2) {
. . .
if (cond3) {
. . .
}
}
}
return nullptr;
```
can be refactored into:
```
if (!cond1)
return nullptr;
. . .
if (!cond2)
return nullptr;
. . .
if (!cond3)
return nullptr;
. . .
```
It is easier to follow the logic if the function is composed in this manner
because from the very beginning you know that `else` with more stuff is not
going to follow.
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
