kbobyrev added a comment.
Thanks! The code looks right, my comments are mostly for the comments around to
validate my mental model of the code structure and behavior.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:17413
+ StringRef Separator = LSI->NumExplicitCaptures > 0 ? ", " : "";
+ if (Var->getDeclName().isIdentifier() && !Var->getName().empty()) {
+ SourceLocation VarInsertLoc = LSI->IntroducerRange.getEnd();
----------------
Would be useful to have high-level comments for all three cases you have here.
This one is for individual variables, right?
================
Comment at: clang/lib/Sema/SemaExpr.cpp:17422
+ << Var << /*reference*/ 1
+ << FixItHint::CreateInsertion(VarInsertLoc, FixBuffer);
+ }
----------------
Not having `return;` after this one or the other ones throws me off-guard and I
came to the realization that what you intend here is: the function can issue
all `FixItHint`s - for capturing a variable by value, reference and default
captures by value and reference. So, in total there could be 4 `FixItHint` so
that the user could choose between all of those and chose the one they want. Is
that correct? I think this is something worth documenting.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:17425
+
+ // Default capture mode must be specified first.
+ SourceLocation DefaultInsertLoc =
----------------
This comment seems a bit off to me? Could you please expand? (I also don't
understand how this relates to the next line).
================
Comment at: clang/lib/Sema/SemaExpr.cpp:17620
Diag(Var->getLocation(), diag::note_previous_decl) << Var;
- if (cast<LambdaScopeInfo>(CSI)->Lambda)
+ if (cast<LambdaScopeInfo>(CSI)->Lambda) {
Diag(cast<LambdaScopeInfo>(CSI)->Lambda->getBeginLoc(),
----------------
now there are 3 `cast<LambdaScopeInfo>`, maybe introduce a variable? would've
been much better with C++ init + condition but still
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96975/new/
https://reviews.llvm.org/D96975
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
