[PATCH] D96975: [Sema] Add some basic lambda capture fix-its

[Kirill Bobyrev via Phabricator via cfe-commits]

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