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 on this!
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:182-184
+ if (InsertionPoint->Parent->ASTNode.get<ParmVarDecl>() != nullptr) {
+ return false;
+ }
----------------
5chmidti wrote:
> nridge wrote:
> > 5chmidti wrote:
> > > This is supposed to stop the following invalid code from happening:
> > > ```
> > > void foo() {
> > > int placeholder = 42;
> > > [](int x = placeholder {};
> > > extern void bar(int x = placeholder);
> > > }
> > > ```
> > >
> > > clangd does not seem to support extracting from the initializers of
> > > defaulted parameters, should I keep the condition as is, or should I do
> > > something different here? It is legal to have default arguments in global
> > > scope (examples below).
> > >
> > > The following insertions could exist (out of scope for this patch):
> > > ```
> > > int placeholder = 42;
> > > void foo() {
> > > [](int x = placeholder {};
> > > extern void bar(int x = placeholder);
> > > }
> > > ```
> > > ```
> > > struct X {
> > > static inline int placeholder = 42;
> > > void foo(int x = placeholder) {}
> > > };
> > > ```
> > >
> > > Either way, I'll have to adjust the comment because this is not just to
> > > stop default parameter initializers in lambdas from being extracted to a
> > > local scope.
> > I'm having trouble following this comment. Can you give the testcase that
> > would produce this invalid code?
> I provided test cases in lines 147-150 in ExtractVariableTests.cpp. For the
> example given in my comment, the test case would be:
> Pre:
> ```
> void foo() {
> [](int x = [[42]]) {};
> }
> ```
> (erroneous) Post:
> ```
> void foo() {
> int placeholder = 42;
> [](int x = placeholder) {};
> }
> ```
>
Thanks. I think the reason for my confusion was that the code snippet in your
original comment was missing a ')`, and I thought perhaps the refactoring
introduced that syntax error, but I understand now that the actual issue is a
semantic error where a default argument references a local variable which isn't
allowed.
I think this is fine, the only cleanup I think is needed here is to collapse
the "Allow all expressions..." comment at the top of the block, and the "Allow
expressions..." comment just below, into one.
================
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 important are the variables captured
and
----------------
5chmidti wrote:
> nridge wrote:
> > Looking at
> > [RecursiveASTVisitor::TraverseLambdaExpr](https://searchfox.org/llvm/rev/094539cfcb46f55824402565e5c18580df689a67/clang/include/clang/AST/RecursiveASTVisitor.h#2652-2691),
> > there are a few things it traverses in addition to the lambda's parameters
> > and body (which we are saying are ok to skip) and the lambda's captures
> > (which we are traversing).
> >
> > For example, the lambda may have an explicit result type which references a
> > variable in scope:
> >
> > ```
> > int main() {
> > if (int a = 1)
> > if ([[ []() -> decltype(a){ return 1; } ]] () == 4)
> > a = a + 1;
> > }
> >
> > ```
> >
> > Here, extracting the lambda succeeds, and the reference to `a` in
> > `decltype(a)` becomes dangling.
> I'll update the diff when I've implemented this. A requires clause may
> reference a variable like `a` as well.
> A requires clause may reference a variable like `a` as well.
Technically, an explicit template parameter list can also reference a local
variable via e.g. a default argument for a non-type parameter.
I appreciate that we're getting into increasingly obscure edge cases here, so
please feel free to use your judgment and draw a line somewhere; the
refactoring introducing a compiler error when invoked on some obscure/unlikely
piece of code is not that big of a deal.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:187
+ // Allow expressions, but only allow completely selected lambda
+ // expressions or unselected lambda expressions that are the parent of
+ // the originally selected node, not partially selected lambda
----------------
I think it's worth adding to the comment *why* we allow unselected lambda
expressions (to allow extracting from capture initializers), as it's not obvious
================
Comment at:
clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp:149
+ [](int x = [[10]]){};
+ [](auto h = [i = [[ [](){} ]]](){}) {};
+ [](auto h = [[ [i = [](){}](){} ]]) {};
----------------
It's easy to overlook the purpose of this line amidst all the brackets, I would
suggest adding a comment like:
```
// Extracting from a capture initializer is usually fine, but not if
// the capture itself is nested inside a default argument
```
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/cgi-bin/mailman/listinfo/cfe-commits
