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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Nice fixes! A few drive-by comments from me, up to you though.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7459-7460
     "capture-default specified">;
+  def note_lambda_variable_capture_fixit : Note<
+    "capture variable %0 by %select{value|reference}1">;
+  def note_lambda_default_capture_fixit : Note<
----------------
njames93 wrote:
> Does the variable name need attaching to the note, given the note is attached to a fix-it containing the name of the variable already?
perhaps not, but I don't think it hurts


================
Comment at: clang/lib/Sema/SemaExpr.cpp:17435
+  // known not to be copy constructible.
+  bool ShouldOfferCopyFix = [&] {
+    // Offer a Copy fix even if the type is dependent.
----------------
this lambda looks like it could/should be a standalone function


================
Comment at: clang/lib/Sema/SemaExpr.cpp:17444
+    if (CXXRecordDecl *RD = T->getAsCXXRecordDecl()) {
+      if (RD->hasSimpleCopyConstructor())
+        return true;
----------------
I suspect you need to handle the case where RD is incomplete


================
Comment at: clang/lib/Sema/SemaExpr.cpp:17453
+    }
+    return T.isPODType(Sema.getASTContext());
+  }();
----------------
POD is rarely precisely the right concept (and deprecated for that reason).
Is isTriviallyCopyableType() just as good here?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:17457
+  SmallString<32> FixBuffer;
+  StringRef Separator = LSI->NumExplicitCaptures > 0 ? ", " : "";
+  if (Var->getDeclName().isIdentifier() && !Var->getName().empty()) {
----------------
This logic assumes there's no default capture, whic his reasonable but not locally obvious - add an assert?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:17489
+                                  : false;
+    // We can't use default capture by copy if any captures already specified
+    // capture by copy.
----------------
While it's technically possible to transform `[&a, &b]` to `[=, &a, &b]`, it seems very unlikely the user actually wants this: we should capture the new variable by copy and make that the default, even though so far we've been listing captures explicitly and by reference.

On the other hand, transforming `[a, b]` to `[=]` seems more useful, but isn't supported.

I'd suggest just leaving both cases out - only offer a default capture if there are no explicit captures already.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96975