vsavchenko added a comment.
Great job on the patch! Thanks!
================
Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:166-167
-static bool isPossiblyEscaped(const VarDecl *VD, ExplodedNode *N) {
+static bool isCapturedByReference(const VarDecl *VD, ExplodedNode *N,
+ const DeclRefExpr *DR) {
+ assert(DR->refersToEnclosingVariableOrCapture());
----------------
One thought here.
I think it's better to not have implicit connections between parameters that
are not obvious to the user of the function (or to the person who is going to
modify this function in the future). Here we always have `VD =
DR->getDecl()->getCanonicalDecl()`. So, IMO, the interface of this function
will be much cleaner if we accept only a `DeclRefExpr`.
================
Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:185-186
+ return FD->getType()->isReferenceType();
+ } else {
+ assert(false && "Unknown captured variable");
+ }
----------------
But actually, it's a bit different with this one. I don't know exact details
and rules how clang sema fills in the class for lambda.
According to [[ https://en.cppreference.com/w/cpp/language/lambda |
cppreference ]]:
> For the entities that are captured by reference (with the default capture [&]
> or when using the character &, e.g. [&a, &b, &c]), it is unspecified if
> additional data members are declared in the closure type
It can be pretty much specified in clang, that's true, but it looks like in
`DeadStoreChecker` we have a very similar situation and we do not assume that
captured variable always have a corresponding field.
================
Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:188-191
+ } else {
+ assert(false &&
+ "Captured variable should only be seen while evaluating a lambda");
+ }
----------------
There is a general rule in LLVM's style guide of trying to minimize nestedness
of `if's, `forms and so on.
In this particular situation this assertion can be more local: `assert(MD &&
MD->getParent()->isLambda() & ...);`.
This way it is more obvious what it checks and we avoid confusion while reading
this function because `if (condition)` automatically means that `!condition` is
something that might happen.
The same applies to another assertion here.
================
Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:202-203
- const bool isParm = isa<ParmVarDecl>(VD);
+ const bool IsEntryValue =
+ isa<ParmVarDecl>(VD) || DR->refersToEnclosingVariableOrCapture();
// Reference parameters are assumed as escaped variables.
----------------
I think this whole section is less obvious now and requires one good solid
comment explaining what are the different situations here, what is "entry" and
so on.
================
Comment at: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp:204
+ isa<ParmVarDecl>(VD) || DR->refersToEnclosingVariableOrCapture();
// Reference parameters are assumed as escaped variables.
+ if ((DR->refersToEnclosingVariableOrCapture() &&
----------------
I think that this comment has to be updated now.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102273/new/
https://reviews.llvm.org/D102273
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
