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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
