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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
