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

Kadir Cetinkaya via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Jul 18 00:55:34 PDT 2019

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;

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...)

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list