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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev accepted this revision.
kbobyrev added a comment.
This revision is now accepted and ready to land.

LGTM, thanks for implementing this and apologies for the review delay (was a tough week for me & my team).

The code is very easy to follow now and the only suggestions are stylistic nits.



================
Comment at: clang/lib/Sema/SemaExpr.cpp:17431
+static void buildLambdaCaptureFixit(Sema &Sema, LambdaScopeInfo *LSI,
+                                    VarDecl *Var) {
+  // Don't offer Capture by copy of default capture by copy fixes if Var is
----------------
This code heavily relies on not having any default captures and I can see that all of the code paths are correct (this condition is checked before calling the function), but I think it's better to explicitly check this with an assert.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:17439
+    auto T = Var->getType();
+    // capturing a reference type by copy will still copy the underlying type.
+    if (T->isReferenceType())
----------------



================
Comment at: clang/lib/Sema/SemaExpr.cpp:17445
+        return true;
+      if (RD->hasUserDeclaredCopyConstructor()) {
+        for (CXXConstructorDecl *CT : RD->ctors()) {
----------------
nit: the style seems to be omitting `{}` for single-line sequences of if/for/whiles, probably remove in this case?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:17446
+      if (RD->hasUserDeclaredCopyConstructor()) {
+        for (CXXConstructorDecl *CT : RD->ctors()) {
+          if (CT->isCopyConstructor())
----------------
nit: `CT` -> `Ctor`? `CT` looks a little unexpected.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:17487
+      if (Sema.getLangOpts().CPlusPlus17)
+        CanDefaultCopyCapture = LSI->getCXXThisCapture().isCopyCapture();
+      else
----------------
also allows removing enclosing `{}`


================
Comment at: clang/lib/Sema/SemaExpr.cpp:17495
+      // capture by copy.
+      if (llvm::none_of(LSI->Captures, [](Capture &C) {
+            return !C.isThisCapture() && !C.isInitCapture() &&
----------------
maybe just pull the outside if into the inner one? does not seem to hurt readability and reduces the indentation + LOC number.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96975