[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: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