[PATCH] D122698: [clangd] Add support to extract method for ExtractFunction Tweak

FabioRS added a comment. In D122698#3430211 , @sammccall wrote: > You're right about the out-of-line function case. Current version looks great! > I had to rework the logic around merging the edits a little, it wasn't quite > correct and was hitting asse

[PATCH] D122698: [clangd] Add support to extract method for ExtractFunction Tweak

sammccall added a comment. You're right about the out-of-line function case. Current version looks great! I had to rework the logic around merging the edits a little, it wasn't quite correct and was hitting assertions (not sure if you were seeing these locally, but phabricator is not currently r

[PATCH] D122698: [clangd] Add support to extract method for ExtractFunction Tweak

This revision was automatically updated to reflect the committed changes. Closed by commit rGa0e4ba4b4607: [clangd] Add support to extract method for ExtractFunction Tweak (authored by FabioRS, committed by sammccall). Changed prior to commit: https://reviews.llvm.org/D122698?vs=420336&id=42057

[PATCH] D122698: [clangd] Add support to extract method for ExtractFunction Tweak

FabioRS updated this revision to Diff 420336. FabioRS added a comment. Thanks! I removed the excess setting/resseting of the SyntacticDC and SemanticDC ExtractedFunc.SyntacticDC = ExtZone.EnclosingFunction->getLexicalDeclContext(); ExtractedFunc.SemanticDC = ExtZone.EnclosingFunction->

[PATCH] D122698: [clangd] Add support to extract method for ExtractFunction Tweak

FabioRS added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:765 + + ExtractedFunc.ForwardDeclarationSyntacticDC = ExtractedFunc.SemanticDC = + ExtractedFunc.SyntacticDC = ExtZone.EnclosingFunction->getDeclContext(); --

[PATCH] D122698: [clangd] Add support to extract method for ExtractFunction Tweak

FabioRS marked 2 inline comments as done. FabioRS added a comment. In D122698#3426237 , @sammccall wrote: > Thanks, this looks great! Just a couple of nits left. > Appreciate you fixing the out-of-line `ns::f()` case too. It's easier to > understand the

[PATCH] D122698: [clangd] Add support to extract method for ExtractFunction Tweak

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thanks, this looks great! Just a couple of nits left. Appreciate you fixing the out-of-line `ns::f()` case too. It's easier to understand the fixed logic than the broken logic. I guess y

[PATCH] D122698: [clangd] Add support to extract method for ExtractFunction Tweak

FabioRS updated this revision to Diff 419868. FabioRS marked an inline comment as done. FabioRS added a comment. I did some simple refactor on the code I did for the function-method and it fixed the namespace syntactic problem too. I am not sure if it expands the pull request scope. CHANGES SI

[PATCH] D122698: [clangd] Add support to extract method for ExtractFunction Tweak

FabioRS updated this revision to Diff 419616. FabioRS marked 6 inline comments as done. FabioRS set the repository for this revision to rG LLVM Github Monorepo. FabioRS added a comment. namespace ns { int y = 1; void foo(); } void ns::foo() {[[int x; y++; ]]} namespace ns { int y = 1; voi

[PATCH] D122698: [clangd] Add support to extract method for ExtractFunction Tweak

sammccall added a comment. OK, I think the main remaining thing is enclosing contexts... (sorry, I wasn't thinking clearly about these in the last round, and I think existing code gets this slightly wrong too). Each declaration has potentially *two* contexts, semantic and syntactic, and the di

[PATCH] D122698: [clangd] Add support to extract method for ExtractFunction Tweak

FabioRS added a comment. In D122698#3418018 , @sammccall wrote: > Thanks! I'll look at the changes in the morning, just wanted to mention one > thing > > In D122698#3417946 , @FabioRS wrote: > >> I can not run th

[PATCH] D122698: [clangd] Add support to extract method for ExtractFunction Tweak

sammccall added a comment. Thanks! I'll look at the changes in the morning, just wanted to mention one thing In D122698#3417946 , @FabioRS wrote: > I can not run the test the consteval, so it is not in the diff. > > TestTU failed to build (suppress wi

[PATCH] D122698: [clangd] Add support to extract method for ExtractFunction Tweak

FabioRS updated this revision to Diff 419296. FabioRS added a comment. Full patch CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122698/new/ https://reviews.llvm.org/D122698 Files: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp clang-tools-extra/clangd/unittests/tweaks

[PATCH] D122698: [clangd] Add support to extract method for ExtractFunction Tweak

FabioRS updated this revision to Diff 419290. FabioRS marked 14 inline comments as done. FabioRS added a comment. I am not sure about the LangOptions object, the NestedNameSpecifier needs it to correctly print the nested classes-name and not sure about the getEnclosing() too. I can not run the

[PATCH] D122698: [clangd] Add support to extract method for ExtractFunction Tweak

sammccall added a comment. Thanks for doing this, it looks great! A few comments on: - a few on cases that I think aren't handled quite right - structure of the new code - structure of the *old* code: names etc that no longer fit after these changes There's also the ugliness around merging edit

[PATCH] D122698: [clangd] Add support to extract method for ExtractFunction Tweak

FabioRS created this revision. FabioRS added a reviewer: sammccall. FabioRS added a project: clang-tools-extra. Herald added subscribers: usaxena95, kadircet, arphaman. Herald added a project: All. FabioRS requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, ilya-biryu