This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Extract Function: add hoisting support
Needs ReviewPublic

Authored by 5chmidti on Nov 22 2022, 7:28 AM.



Adds support to hoist variables declared inside the selected region
and used afterwards back out of the extraced function for later use.
Uses the explicit variable type if only one decl needs hoisting,
otherwise pair or tuple with auto return type deduction
(requires c++14) and a structured binding (requires c++17) or
explicitly unpacking the variables with get<>.

Diff Detail

Event Timeline

5chmidti created this revision.Nov 22 2022, 7:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 22 2022, 7:28 AM
5chmidti requested review of this revision.Nov 22 2022, 7:28 AM
5chmidti updated this revision to Diff 477184.Nov 22 2022, 7:33 AM

Fixup: rm added includes

5chmidti updated this revision to Diff 477204.Nov 22 2022, 8:08 AM

Fix windows build by setting the c++ standard for the hoisting tests explicitly to c++17.

nridge added a subscriber: nridge.Nov 26 2022, 10:50 PM
5chmidti updated this revision to Diff 538286.Jul 7 2023, 3:42 PM

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 NamedDecls to be hoisted is a TypeDecl. The HoistedSet can contain TypeDecls, 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 IncludeCleaners 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)?
5chmidti planned changes to this revision.Jul 22 2023, 1:40 PM

I found a new issue. I'll probably fix it by next weekend.

5chmidti requested review of this revision.Jul 22 2023, 4:28 PM

Never mind, the problem is unrelated to this patch. Filed clangd issue #1710

Ping. Because Phabricator is set to be made read-only on the 1. of October, it might be better to not review this patch here and instead open it on GitHub instead. Thoughts?