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