[PATCH] D138499: [clangd] Extract Function: add hoisting support

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti updated this revision to Diff 538286.
5chmidti added a comment.

Ping, and:

- change find_if of post-use detection to range-for with a filtered range
- make `renderHoistedCall` a private member of `NewFunction`
- remove the `Render` lambda in `renderHoistSet` in favor of putting the body into the loop, improving readability imo.
- swapped order in tertiary operator to remove the `!` in the condition for better readability
- add abort condtition if any of the `NamedDecl`s to be hoisted is a `TypeDecl`. The `HoistedSet` can contain `TypeDecl`s, which block the extraction because types cannot be hoisted as easily as variables. Hoisting types could be done by moving the declaration into the scope that the functions are declared in (global, namespace or records). I could take a look at this if there is interest, but I think that should be its own diff.
- add tests about full and partial selection of type aliases and classes

Open questions:

- I think that `<tuple>`/`<utility>` should be included if a `tuple` or `pair` is used, but couldn't figure out a clean way to include the headers. It looks like the way to go would be through `IncludeCleaner`s `insert` function, but for that I woul dneed to construct the `IncludeInserter`. I don't think I can get the `FormatStyle` or `BuildDir` from inside of a tweak. Does anyone have an idea or is this a non-issue?
- The tweak is unavailable if a tuple or pair is required for the hoisting, but no auto return-type deduction is available. This is the case because I implemented the return type of the hoisted variables to use `tuple` or `pair` if two or more variables are returned, but to keep the return type short, I set the return type in these cases to `auto`. Should the return type be fully spelled out? Should it be spelled out always or should there be a config option (seems a bit overkill)?
- I'm not a fan of the `const&` `HoistSet` member that I used. Should I change this to a pointer (its always non-null)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138499

Files:
  clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
  clang-tools-extra/docs/ReleaseNotes.rst

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D138499.538286.patch
Type: text/x-patch
Size: 19577 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20230707/73a8e499/attachment-0001.bin>