[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:195
   Intent intent() const override { return Refactor; }
+  // Compute the extraction context for the Selection
+  void computeExtractionContext(const SelectionTree::Node *N,
----------------
comment seems to be repeating the code, instead lets comment about the assumptions, like not working on declrefexpr's etc.


================
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;
----------------
maybe instead of checking internals like `Target` just make `computeExtractionContext` return an `llvm::Error` and check for success?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:228
+const SelectionTree::Node *
+getParentExprOfType(const SelectionTree::Node *Node) {
+  for (auto *ParNode = Node->Parent; ParNode && ParNode->ASTNode.get<Expr>();
----------------
I don't think traversing the tree without any conditions is a good idea, for example:

```
struct Foo {
int bar(int);
int z;
};

void foo() {
Foo f;
f.bar([[f.z]]);
}
```

above selection is on a memberexpr, that has a parent membercallexpr but it is actually not related to selection at all but I believe we'll extract the whole thing in that case.
I don't think that is the intended behavior, could you also add a test case for that?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:270
+  // look for the call expr parent if the selected expr is a MemberExpr
+  if (dyn_cast_or_null<clang::MemberExpr>(SelectedExpr)) {
+    TargetNode = getParentExprOfType<CXXMemberCallExpr>(N);
----------------
nit: braces


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:273
+  }
+  if (TargetNode && canBeAssigned(TargetNode))
+    Target = llvm::make_unique<ExtractionContext>(TargetNode, SM, Ctx);
----------------
nit: instead of checking success check failure and bail out?

```
if(!TargetNode || !canBeass...)
  return;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64717