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