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

Updated std::Option, std::Either and std::Result #8268

Closed
wants to merge 1 commit into from

Conversation

Kimundi
Copy link
Member

@Kimundi Kimundi commented Aug 3, 2013

  • Lifted the quality of the either and result module to that of option
  • Made naming schemes consistent between Option, Result and Either

I also touched up on two issues that haven't yet reached a real consensus, so it wouldn't wonder me if this PR doesn't go through uncontested:

Todo:

Adding testcases for all function in the three modules. Even without the few functions I added, the coverage wasn't complete to begin with. But I'd rather do that as a follow up PR, I've touched to much code here already, need to go through them again with a clear mind.

Made naming schemes consistent between Option, Result and Either
Changed Options Add implementation to work like the maybe monad (return None if any of the inputs is None)
Renamed Option::unwrap to Option::get and Option::take_unwrap to Option::take_get
@bblum
Copy link
Contributor

bblum commented Aug 3, 2013

Not sure I agree with the renaming of unwrap -- I prefer get (and other operations with no suffix, like iter), to mean by-reference. At least there did not seem to be consensus, so deciding on one seems a little premature.

@lilyball
Copy link
Contributor

lilyball commented Aug 3, 2013

The naming RFC #7887 seems to suggest that get() returns a ref and that things like Option should continue to use the name unwrap() for consumption. So I'm a bit confused here.

@brson
Copy link
Contributor

brson commented Aug 3, 2013

I killed this build. Sorry. There have been some concerns raised here and on IRC, from reading the referenced bug it's not clear that a consensus was reached, and this is a very significant design decision.

@Kimundi
Copy link
Member Author

Kimundi commented Aug 6, 2013

With #8288 merged, this can be closed now.

@Kimundi Kimundi closed this Aug 6, 2013
bors added a commit that referenced this pull request Aug 7, 2013
According to #7887, we've decided to use the syntax of `fn map<U>(f: &fn(&T) -> U) -> U`, which passes a reference to the closure, and to `fn map_move<U>(f: &fn(T) -> U) -> U` which moves the value into the closure. This PR adds these `.map_move()` functions to `Option` and `Result`.

In addition, it has these other minor features:
 
* Replaces a couple uses of `option.get()`, `result.get()`, and `result.get_err()` with `option.unwrap()`, `result.unwrap()`, and `result.unwrap_err()`. (See #8268 and #8288 for a more thorough adaptation of this functionality.
* Removes `option.take_map()` and `option.take_map_default()`. These two functions can be easily written as `.take().map_move(...)`.
* Adds a better error message to `result.unwrap()` and `result.unwrap_err()`.
osaut pushed a commit to osaut/rust that referenced this pull request Oct 31, 2013
This is an alternative version to rust-lang#8268, where instead of transitioning to `get()` completely, I transitioned to `unwrap()` completely.

My reasoning for also opening this PR is that having two different functions with identical behavior on a common datatype is bad for consistency and confusing for users, and should be solved as soon as possible. The fact that apparently half the code uses `get()`, and the other half `unwrap()` only makes it worse.

If the final naming decision ends up different, there needs to be a big renaming anyway, but until then it should at least be consistent.

---

- Made naming schemes consistent between Option, Result and Either
- Lifted the quality of the either and result module to that of option
- Changed Options Add implementation to work like the maybe Monad (return None if any of the inputs is None)  
  See rust-lang#6002, especially my last comment.
- Removed duplicate Option::get and renamed all related functions to use the term `unwrap` instead  
  See also rust-lang#7887.

Todo: 

Adding testcases for all function in the three modules. Even without the few functions I added, the coverage wasn't complete to begin with. But I'd rather do that as a follow up PR, I've touched to much code here already, need to go through them again later.
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jan 13, 2022
Fix `deref_addrof`

fixes rust-lang#8247

This would supersede rust-lang#8259

changelog: Don't lint `deref_addrof` when the dereference and the borrow occur in different contexts
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.

None yet

6 participants