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<>.
Details
Details
- Reviewers
sammccall
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
Fix windows build by setting the c++ standard for the hoisting tests explicitly to c++17.
Comment Actions
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)?
Comment Actions
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?