[PATCH] D64717: [Clangd] Fixed ExtractVariable for MemberExprs and Assignment Exprs

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:208
   const SelectionTree::Node *N = Inputs.ASTSelection.commonAncestor();
-  if (!N)
-    return false;
-  Target = llvm::make_unique<ExtractionContext>(N, SM, Ctx);
-  return Target->isExtractable();
+  computeExtractionContext(N, SM, Ctx);
+  return Target && Target->InsertionPoint;
----------------
kadircet wrote:
> SureYeaah wrote:
> > kadircet wrote:
> > > maybe instead of checking internals like `Target` just make `computeExtractionContext` return an `llvm::Error` and check for success?
> > Should it instead return a bool since there's actually no error?
> Yes that's also plausible
i was trying to say:

having a function named `computeExtractionContext` that will require you to check some other state even when it returns true sounds like a bad idea.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:292
+  Target = llvm::make_unique<ExtractionContext>(TargetNode, SM, Ctx);
+  return true;
+}
----------------
SureYeaah wrote:
> kadircet wrote:
> > `return Target->InsertionPoint`?
> Changed to checking if target is extractable.
any reasons for not returning `Target->isExtractable()` ?


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:304
       // return statement
-      return ^1;
+      return [[[[t.bar]](t.z)]];
     }
----------------
kadircet wrote:
> another case on bar ?
> 
> ```
> [[[[t.b[[a]]r]](t.z)]]
> ```
i suppose this is done


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:427
            R"cpp(void f(int a) {
                     auto dummy = 1; label: [ [gsl::suppress("type")] ] for (;;) a = dummy;
                  })cpp"},
----------------
I think `auto dummy = 1;` should be within the label. otherwise we might break codes like(which is basically anything with labels):
```
void foo() {
goto label;
label:
 a = 1;
}
```

I don't think it is that important though, and have no idea about the effort necessary feel free to just add a FIXME if it turns out hard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64717