[PATCH] D131067: [analyzer] Treat values passed as parameter as escaped

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

Please, consider stating the motivation behind this change.
For me, by looking at the modified test, it feels like we would lose legitimate findings (aka. true-positive reports) by applying this change.
>From my perspective, the store to `i` is //dead//, since it will be immediately overwritten in the `referenceParameters()` function.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp:536-537
+  void findReferenceParameter(const FunctionDecl *FD, const Expr *const *Args) {
+    unsigned numParams = FD->getNumParams();
+    for (unsigned i = 0; i < numParams; ++i) {
+      if (FD->getParamDecl(i)->getType()->isReferenceType()) {
----------------
You don't seem to use the index, only for iterating over the parameters.
I believe, `FD->parameters()` would simplify this code.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp:538-545
+      if (FD->getParamDecl(i)->getType()->isReferenceType()) {
+        if (auto *DRE = dyn_cast<DeclRefExpr>(Args[i])) {
+          if (auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
+            Escaped.insert(VD);
+          }
+        }
+      }
----------------
By looking at e.g. `findLambdaReferenceCaptures()`, it seems like we should prefer `continue` statements for filtering; probably to avoid such deeply nested intendations.
Along with that, I think the llvm style guide prefers not wrapping single statement branches by courly braces.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp:549
+  void findCallReferenceParameters(const CallExpr *CE) {
+    if (auto *FD = dyn_cast<FunctionDecl>(CE->getCalleeDecl())) {
+      findReferenceParameter(FD, CE->getArgs());
----------------
I'm not sure `getCalleeDecl()` always returns a valid pointer.
I believe the `dyn_cast_or_null` would be more appropriate in this case.
This theory is underpinned by the other uses of the `getCalleeDecl()` API.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp:555
+  void findConstructorReferenceParameters(const CXXConstructExpr *CE) {
+    findReferenceParameter(CE->getConstructor(), CE->getArgs());
+  }
----------------
Why don't you use the `CE->arguments()` method instead? That would result in a range (begin + end).
I believe that is superior in most cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131067