[PATCH] D102273: [analyzer] LoopUnrolling: fix crash when a loop counter is captured in a lambda by reference

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Thanks for addressing my comments! I still have some left though



================
Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:171
+  const Decl *D = LocCtxt->getDecl();
+  const auto *MD = dyn_cast_or_null<CXXMethodDecl>(D);
+  assert(MD && MD->getParent()->isLambda() &&
----------------
`dyn_cast_or_null` also tells the reader that `D` can be null and can be not `CXXMethodDecl`.  So it's better to use `cast` here if you are sure about `D` and have assertions about it.


================
Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:175-178
+  const CXXRecordDecl *CXXRec = MD->getParent();
+  llvm::DenseMap<const VarDecl *, FieldDecl *> LambdaCaptureFields;
+  FieldDecl *LambdaThisCaptureField;
+  CXXRec->getCaptureFields(LambdaCaptureFields, LambdaThisCaptureField);
----------------
Body of this function seems to be too dense.  It is usually hard to read something that doesn't have rest places for eyes.  You can think of it as paragraphs in the written text.
In this particular situation, you can split all these statements and declarations in groups of 3-4.  It would be perfect if they are grouped semantically (like good paragraphs).


================
Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:179
+  CXXRec->getCaptureFields(LambdaCaptureFields, LambdaThisCaptureField);
+  const VarDecl *VD = dyn_cast<VarDecl>(DR->getDecl()->getCanonicalDecl());
+  assert(VD);
----------------
We can just do `cast` here


================
Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:185-186
+    return FD->getType()->isReferenceType();
+  } else {
+    assert(false && "Unknown captured variable");
+  }
----------------
I still think that we should do something about this `else`


================
Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:192-193
+
+// A loop counter is considered escaped if:
+// - It is a global variable.
+// - It is an entry value: a reference parameter or a reference capture.
----------------
I think it's a great idea to list all the cases.  I think we can go even further: enumerate them and actually pinpoint in the implementation where we cover which case.  With numbers it can be much easier.


================
Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:194
+// - It is a global variable.
+// - It is an entry value: a reference parameter or a reference capture.
+// - It is assigned to a non-const reference variable or parameter.
----------------
Can we not introduce new terms and simply call it a **reference**?


================
Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:197
+// - Has its address taken.
+static bool isPossiblyEscaped(const VarDecl *VD, ExplodedNode *N,
+                              const DeclRefExpr *DR) {
----------------
This should be also modified then


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102273