Skip to content
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

Remove a box from future, and add future::unwrap #4020

Closed
wants to merge 2 commits into from

Conversation

erickt
Copy link
Contributor

@erickt erickt commented Nov 22, 2012

This patch removes the box that holds the forced value, which seems unnecessary to me now that moves work well. Along with that I added a way to unwrap the forced value to match option and result.

The one change I'm not positive about is erickt@ae8253c#L0L140. @nikomatsakis: Your name is attached to this change. Does this look right?

@graydon
Copy link
Contributor

graydon commented Nov 22, 2012

The removal of the box seems ... safe-ish to me. Or rather, it seems like it is as safe as the existing code. But that existing pattern looks to me like it becomes unsafe if you add an unwrap() call. Specifically, get_ref looks like it lies about having something that stays alive as long as the future (the unsafe block there). This is not a bad approximation when futures are monotonic from evaluating => forced. But once you can reset one, all bets are off. Consequently I think this should possibly be done at a different layer; or else get_ref removed and changed to a version that takes a callback and does not widen the reference lifetime. I'll tinker with it.

@graydon
Copy link
Contributor

graydon commented Nov 22, 2012

Oh. There's already with. There's a lot of redundancy in this module! I'll tidy it up.

@graydon
Copy link
Contributor

graydon commented Nov 22, 2012

Hm Hm! The more I think about this the more unusual it seems. What's the point of a future that resets itself? You can only read it once, it's equivalent to a port. Also, you're not resetting it here, you're setting it to Evaluating; that means it's wedged and will fail if anyone tries to read it again. I think this is overall not the approach you want; with() for read-access and get() for copy-access are the thing we want. I'll open a separate pull req to tidy those up (removing redundancy) and close this. Reopen if you disagree.

@graydon graydon closed this Nov 22, 2012
@erickt
Copy link
Contributor Author

erickt commented Nov 24, 2012

(Reopening...) I still think this could be pretty useful, it would in effect be a one-shot port. Also, future::unwrap isn't resetting the future, it's consuming it since the future isn't copyable. Evaluating is just an intermediate state to protect against forcing a partially forced future. Perhaps using something like mutable::Mut or std::cell is better than this intermediate state.

#4023 is related.

@erickt erickt reopened this Nov 24, 2012
@nikomatsakis
Copy link
Contributor

FWIW, this change looks safe to me.

@brson
Copy link
Contributor

brson commented Nov 25, 2012

I agree unwrap is safe and should probably exist, though I can't think of a use case where I would prefer future + unwrap over a pipe::oneshot.

@erickt
Copy link
Contributor Author

erickt commented Nov 25, 2012

I wasn't aware of pipe::oneshot, which seems to be almost identical to futures with an unwrap. Can we merge them? Is there any advantage to keeping two very similar constructs?

@brson
Copy link
Contributor

brson commented Nov 26, 2012

The difference is that futures contain a value and pipes produce a value, but they are very similar (and I think the preferred way to create a future now is probably from a oneshot pipe, which also incidentally acts as a 'promise' - if nobody sends the Future value then requesting the value fails).

I do think they have different use cases.

The thing that is making Futures unuseful to me is that they are not Const. For the things where I need futures the most I need them to be sharable. Conceptually I need an ARC<Future>.

jesse99 and others added 2 commits November 26, 2012 21:49
… to Tm and Tm_.

Not entirely clear what the best way to do this is. Right now we persist the entire
struct which seems to be both portable and exactly round-trippable.
This avoids rust-lang#4044 by not using the enum wrapper, and turning Tm_
directly into a struct. Along the way it modernizes the codebase
to eliminate no-implicit-copies warnings.
@graydon
Copy link
Contributor

graydon commented Dec 3, 2012

@erickt is there any reason to keep this open, given pipe::oneshot and the cleanup over in #4023?

@erickt
Copy link
Contributor Author

erickt commented Dec 3, 2012

Nope, closing.

@erickt erickt closed this Dec 3, 2012
bors pushed a commit to rust-lang-ci/rust that referenced this pull request May 15, 2021
RalfJung added a commit to RalfJung/rust that referenced this pull request Nov 10, 2024
pthread-sync test: use thread::scope for more reliable thread scoping
mati865 pushed a commit to mati865/rust that referenced this pull request Nov 12, 2024
pthread-sync test: use thread::scope for more reliable thread scoping
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants