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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added inline comments.


================
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") {
----------------
vsavchenko wrote:
> Is there any reason it's not a method of `FindWhereConstrained`?
Not really.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:179-180
+              // P.get()
+              if (const auto *InnerCastExpr =
+                      llvm::dyn_cast<ImplicitCastExpr>(Sub)) {
+                Sub = InnerCastExpr->getSubExpr();
----------------
vsavchenko wrote:
> I think it's better to `IgnoreParensAndCasts` instead of manual traversal.
What is IgnoreParensAndCasts`? I didn't find it in the source code anywhere (ripgrep, that is).


================
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)) {
----------------
vsavchenko wrote:
> So, situations like `int *a = nullptr, *b = smart.get();` are not supported?
No it works even in that case (I have added a test for that). It's got to do with how the AST data structures are (`int *a = nullptr, *b = smart.get();` is considered a single decl).


================
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()) {
----------------
vsavchenko wrote:
> `llvm::find_if`
Not sure if that'll work neatly since I actually need the return value of the predicate function (the report).


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:238-245
+                }
+              }
+            }
+          }
+        }
+      }
+    }
----------------
vsavchenko wrote:
> 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.
Do you still think that's the case now? (After breaking it into functions).
I also think that for the sort of pattern matching we are doing in this patch, the nested if's make more sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97183