[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;
----------------
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


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:292
+  Target = llvm::make_unique<ExtractionContext>(TargetNode, SM, Ctx);
+  return true;
+}
----------------
`return Target->InsertionPoint`?


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:304
       // return statement
-      return ^1;
+      return [[[[t.bar]](t.z)]];
     }
----------------
another case on bar ?

```
[[[[t.b[[a]]r]](t.z)]]
```


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:349
+      } t;
+      return t.bar([[t.z]]);
     }
----------------
also on t:
```
return [[t]].bar([[[[t]].z]]);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64717