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

Add a try_with method to LocalKey #2030

Closed
wants to merge 2 commits into from

Conversation

PlasmaPower
Copy link

Rendered

You probably know LocalKey as being the creation of the thread_local! macro.

This is my first RFC, so let me know if I'm doing anything wrong.

@nrc nrc added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Jun 15, 2017
@PlasmaPower
Copy link
Author

Is it normal for an RFC to go this long without comments? Is there something I can do to help the process?

@pengowen123
Copy link

I think having a try_with method would be useful to some, but I'm not sure how often the existing with method causes problems. It is true that the current situation requires checking twice, but if state is removed, using try_with(|| {}).is_ok() isn't pretty either. Because of this, I think state should stay, but as I said, these situations may not be uncommon, so hopefully someone that uses this feature has better feedback.

@PlasmaPower
Copy link
Author

Another thing I didn't previously consider about the state is that it's thread local, so if you check state a with call is guaranteed to succeed. I'll change the RFC to not deprecate state. There's also not much reason to deprecate state, so I'll stay on the side of ensuring usability.

@PlasmaPower PlasmaPower changed the title Add a try_with method to LocalKey, replacing the existing but unstable state method Add a try_with method to LocalKey Jul 7, 2017
@PlasmaPower
Copy link
Author

It's been another two weeks since a comment - bump again. Should I continue bumping this, or is there just no interest in this? I'd do the implementation.

@glaebhoerl
Copy link
Contributor

The process description has this bullet point:

  • Each pull request will be labeled with the most relevant sub-team, which will lead to its being triaged by that team in a future meeting and assigned to a member of the subteam.

The first part of this has happened but the second has not (and it should, for every RFC). I'm not sure if they're just busy and haven't gotten to it, or if they accidentally overlooked the RFC. In any case I hope they don't mind if I ping them about it: @brson @alexcrichton @sfackler @BurntSushi @Kimundi @dtolnay @aturon [as far as I'm aware I can't ping @rust-lang/libs directly because I'm not on a team myself]

@alexcrichton
Copy link
Member

Thanks for the RFC @PlasmaPower but I'm not sure that this will satisfy all current use cases for the state method. Right now different locations will act differently depending on the state returned from that function, whereas I believe the proposed semantics here would fold the Valid and Uninitialized states together. It seems reasonable to me to add a function to more gracefully handle the Destroyed state but I don't think we'll be able to deprecate state yet.

@PlasmaPower
Copy link
Author

@alexcrichton The RFC as it stands doesn't deprecate state. While I still haven't found a use case for state, I see why there might be one, so the RFC currently keeps it as is.

@alexcrichton
Copy link
Member

If that's the case then this probably doesn't need an RFC and can likely just be a PR. If you'd like you can close this an submit a PR to rust-lang/rust.

@PlasmaPower
Copy link
Author

I was wondering about that! The readme did say additions to std needed an rfc, but I guess this is pretty small. I'll just wrap it into the existing state feature I guess.

bors added a commit to rust-lang/rust that referenced this pull request Jul 13, 2017
Thread local try with

rust-lang/rfcs#2030 was turned into this PR (the RFC was closed, but it looks like just a PR should be good).

See also: state stabilization issue: #27716

`try_with` is used in two places in std: stdio and thread_info. In stdio, it would be better if the result was passed to the closure, but in thread_info, it's better as is where the result is returned from the function call. I'm not sure which is better, but I prefer the current way as it better represents the scope.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants