-
Notifications
You must be signed in to change notification settings - Fork 12.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Stabilize futures_api #59739
Stabilize futures_api #59739
Conversation
Marking as blocked on FCP completion in #59725. |
f48faf9
to
b55c014
Compare
This comment has been minimized.
This comment has been minimized.
// | ||
// FIXME: remove whenever we have a stable way to accept fn pointers from | ||
// const fn (see rfcs/pull/2632) | ||
#[cfg_attr(not(stage0), rustc_allow_const_fn_ptr)] | ||
pub const fn new( | ||
clone: unsafe fn(*const ()) -> RawWaker, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit surprised we're stabilizing this implementation detail, I would've expected an actual trait to be involved, and these fn
pointers to be obtained internally (which I think is allowed? oh but const fn
can't have trait bounds, hmpf).
Also, *const ()
in a public API? Seems a bit off, I thought we were using extern type
pointers nowadays?
Anyway, regarding the matter at hand, which is constructors that happen to have banned types, I think this is how we can allow them without allowing the same types in any non-constructor const fn
: rust-lang/const-eval#19 (comment) (by making them pattern aliases)
It also means calls would be promotable, which is good, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, there is no reason for this to this unsafe.
While the fields of this struct have to be unsafe fn(*const ())
, the arguments could be fn(&mut T)
where the constructor is generic over T
.
Sadly, this can't be directly coupled with RawWaker
's constructor, but that's only because RawWakerVTable
isn't itself generic... RawWaker
could store a RawWakerVTable<SomePrivateExternType>
, and it would all be much safer.
I'm sorry I haven't remarked this before, I kept thinking this "raw vtable" stuff was kept internal so therefore no need to make it more contrived for safety, I guess I was wrong...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bonus: you could pass a closure where a fn(&mut T)
is expected, and it will just work!
So you can add extra behavior or w/e, pretty easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think making the arguments &mut
would make this implementation unsound since it uses the same data pointer for all instances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was already discussed on the RFC (why it isn't a trait and why futures-rs will provide a safe trait for it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cramertj oops, I should've made separate comments, your resolving meant people wouldn't see my approval of rustc_allow_const_fn_ptr
(#59739 (comment))
☔ The latest upstream changes (presumably #59119) made this pull request unmergeable. Please resolve the merge conflicts. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
cb64506
to
bea79a1
Compare
#[stable(feature = "futures_api", since = "1.36.0")] | ||
Ready( | ||
#[stable(feature = "futures_api", since = "1.36.0")] | ||
T |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why we stabilize this field. Is this intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know. Thanks @Nemo157 .
Stabilize futures_api cc rust-lang#59725. Based on rust-lang#59733 and rust-lang#59119 -- only the last two commits here are relevant. r? @withoutboats , @oli-obk for the introduction of `rustc_allow_const_fn_ptr`.
Failed in #60208 (comment), @bors r- |
Updated UI test output. |
📌 Commit 858a8f18e747790646038f505e9adb8dacf5dd7b has been approved by |
@bors r=withoutboats |
📌 Commit 3f966dc has been approved by |
Stabilize futures_api cc rust-lang#59725. Based on rust-lang#59733 and rust-lang#59119 -- only the last two commits here are relevant. r? @withoutboats , @oli-obk for the introduction of `rustc_allow_const_fn_ptr`.
Stabilize futures_api cc rust-lang#59725. Based on rust-lang#59733 and rust-lang#59119 -- only the last two commits here are relevant. r? @withoutboats , @oli-obk for the introduction of `rustc_allow_const_fn_ptr`.
Rollup of 5 pull requests Successful merges: - #56278 (Future-proof MIR for dedicated debuginfo.) - #59739 (Stabilize futures_api) - #59822 (Fix dark css rule) - #60186 (Temporarily accept [i|u][32|size] suffixes on a tuple index and warn) - #60190 (Don't generate unnecessary rmeta files.) Failed merges: r? @ghost
cc #59725.
Based on #59733 and #59119 -- only the last two commits here are relevant.
r? @withoutboats , @oli-obk for the introduction of
rustc_allow_const_fn_ptr
.@XAMPPRocky Prefer T-libs for relnotes.
// Centril