Add support for splatted function pointers - #159643
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
|
@rustbot label +C-bug |
This comment has been minimized.
This comment has been minimized.
|
rustbot has assigned @petrochenkov. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
@rustbot reroll |
Inline splatted_callee() Add tests for generic function pointers
There was a problem hiding this comment.
This PR got quite big and seemingly fixes a bunch of different things, is there any chance you can split it up? At least the refactoring into using the custom enums seems like it could be split out, just stubbing out the branches for FnPtr and errors. If possible a separate PR would be nice (just assign me as a reviewer to preserve context), otherwise separate commits would also work.
It's just really tough to review like this.
|
|
||
| fn main() { | ||
| // FIXME(rustfmt): the attribute gets deleted by rustfmt | ||
| #[rustfmt::skip] |
There was a problem hiding this comment.
Might be better to put the #[rustfmt::skip] on the function to reduce noise?
There was a problem hiding this comment.
Happy to do that. It's better for readability, but slightly worse for maintenance because now every line is unformatted, not just the splatted type lines.
Ultimately we'll fix this in rustfmt if function argument attributes become a thing.
There was a problem hiding this comment.
We usually just use assert_eq!, is there any particular reason to deviate from that? Your generic<T: Tuple + Debug>(#[splat] a: T) could use format! and return a String?
There was a problem hiding this comment.
No particular reason, it will be much more readable to use assert_eq! across all the splat UI tests. The generic case can just return the argument.
A codegen bug could swap the tupled argument and return value, or overwrite the return value, but the multi-argument case will catch that. Or it might be worth having a test that swaps the order of the arguments in the return value. I'll have a think about it.
| receiver: Option<&'tcx hir::Expr<'tcx>>, | ||
| has_receiver: bool, | ||
| // This is the method receiver, or the function path | ||
| receiver_or_func: &'tcx hir::Expr<'tcx>, |
There was a problem hiding this comment.
this still feels kind of wrong, although I don't have any obviously better suggestions.
There was a problem hiding this comment.
To make the states clearer, it could be:
enum SplattedFunc {
FnDefReceiver(Expr),
NoFnDefReceiver,
FnPtrExpression(Expr),
}|
Reminder, once the PR becomes ready for a review, use |
There was a problem hiding this comment.
This PR got quite big and seemingly fixes a bunch of different things, is there any chance you can split it up? At least the refactoring into using the custom enums seems like it could be split out, just stubbing out the branches for FnPtr and errors. If possible a separate PR would be nice (just assign me as a reviewer to preserve context), otherwise separate commits would also work.
That's good feedback. I ended up squashing the whole thing to make it easier to rebase and rewrite, but I didn't split it into commits again.
I think it can be split into something like:
- inline the splatted_callee function
- refactor using custom enums (with stubs)
- support for splatted FnPtrs
- update tests to use assert_eq!
If you'd like some of those commits in different PRs, or in a different order, please let me know.
| receiver: Option<&'tcx hir::Expr<'tcx>>, | ||
| has_receiver: bool, | ||
| // This is the method receiver, or the function path | ||
| receiver_or_func: &'tcx hir::Expr<'tcx>, |
There was a problem hiding this comment.
To make the states clearer, it could be:
enum SplattedFunc {
FnDefReceiver(Expr),
NoFnDefReceiver,
FnPtrExpression(Expr),
}|
|
||
| fn main() { | ||
| // FIXME(rustfmt): the attribute gets deleted by rustfmt | ||
| #[rustfmt::skip] |
There was a problem hiding this comment.
Happy to do that. It's better for readability, but slightly worse for maintenance because now every line is unformatted, not just the splatted type lines.
Ultimately we'll fix this in rustfmt if function argument attributes become a thing.
There was a problem hiding this comment.
No particular reason, it will be much more readable to use assert_eq! across all the splat UI tests. The generic case can just return the argument.
A codegen bug could swap the tupled argument and return value, or overwrite the return value, but the multi-argument case will catch that. Or it might be worth having a test that swaps the order of the arguments in the return value. I'll have a think about it.
Tracking issue: #153629
This PR adds support for splatted function pointers.
Currently splatted function pointers ICE due to unpopulated side-tables. This PR fixes the ICE by populating the side-table correctly, and fixes the MIR lowering code. It also refactors the surrounding code to reduce code duplication.
Generic function pointers also work. I'm sure if there's something extra we need to do to support all generic function pointers?
Close #158603