This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG94b14355e2ef: [clangd] allow extracting to variable for
lambda exp
5chmidti added a comment.
Thank you. Could you please commit this for me with `Julian Schmidt
<44101708+5chmi...@users.noreply.github.com>`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D141757/new/
https://reviews.llvm.org/D141757
__
nridge accepted this revision.
nridge added a comment.
I'm fine with allowing partial selection for consistency with other expressions.
I think this patch is in a good state. Thanks for your patience with the review
:) Let me know if you need me to commit it for you.
Repository:
rG LLVM Gith
5chmidti marked an inline comment as done.
5chmidti added inline comments.
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:187
+// Allow expressions, but only allow completely selected lambda
+// expressions or unselected lambda expression
5chmidti updated this revision to Diff 556349.
5chmidti added a comment.
Fixup to last revision change:
- do not block the partial selection of lambdas
There is no reason to block the partial selection of lambdas. Other expressions
can be partially selected as well and still offer an extraction
5chmidti updated this revision to Diff 556348.
5chmidti added a comment.
addressed comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D141757/new/
https://reviews.llvm.org/D141757
Files:
clang-tools-extra/clangd/refactor/tweaks/ExtractVariabl
nridge added a comment.
(Otherwise the updates look good!)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D141757/new/
https://reviews.llvm.org/D141757
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailm
nridge requested changes to this revision.
nridge added inline comments.
This revision now requires changes to proceed.
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:187
+// Allow expressions, but only allow completely selected lambda
+/
nridge added inline comments.
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:102
+
+ if (clang::Expr *const RequiresClause =
+ LExpr->getTrailingRequiresClause();
nit: just `if (clang::Expr *const RequiresClause =
LExp
5chmidti marked an inline comment as done.
5chmidti added a comment.
Ping
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D141757/new/
https://reviews.llvm.org/D141757
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.or
5chmidti marked 4 inline comments as done.
5chmidti added inline comments.
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:91
+
+// Local variables declared inside of the selected lambda cannot go out of
+// scope. The DeclRefExprs that are import
5chmidti updated this revision to Diff 552065.
5chmidti added a comment.
Addressed comments:
- added explicit template parameters, trailing return-type declarations and
attributes to the visitor that finds DeclRefExprs
- added a few more comments about why an extraction is expected to be
unavai
nridge requested changes to this revision.
nridge added a comment.
This revision now requires changes to proceed.
Ok, I've finished looking through the patch; sorry it took so long to get to.
It generally looks pretty good to me, I just have some fairly minor comments.
Thanks again for your work
5chmidti marked an inline comment as done.
5chmidti added inline comments.
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:91
+
+// Local variables declared inside of the selected lambda cannot go out of
+// scope. The DeclRefExprs that are import
nridge added a comment.
Haven't looked at everything yet, but wanted to mention one thing I noticed.
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:91
+
+// Local variables declared inside of the selected lambda cannot go out of
+// scope. The
nridge added a comment.
I promise I haven't forgotten about this. It's just been a challenge finding a
large enough chunk of time to page in all the relevant context and think
through the issues. Maybe this weekend...
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://r
5chmidti added a comment.
Ping
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D141757/new/
https://reviews.llvm.org/D141757
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cg
5chmidti added a comment.
Ping
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D141757/new/
https://reviews.llvm.org/D141757
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cg
5chmidti updated this revision to Diff 538289.
5chmidti added a comment.
Ping, and:
- removed mention of captures from the condition about blocking extraction
because that is allowed
- removed `(of a lambda)` from the following comment because it is not just
about defaulted parameters of lambda
5chmidti added inline comments.
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:178
+ // Allow all expressions except partial LambdaExpr selections since we
+ // don't want to extract from the captures/default arguments of a lambda
+ if (is
20 matches
Mail list logo
