Provide Entry-like API for Option #39289

shahn · 2017-01-25T06:18:05Z

This implements #39288.

I am wondering whether to use std::intrinsics::unreachable!() here. Both seems fine to me (the second match optimizes away in release mode).

rust-highfive · 2017-01-25T06:18:19Z

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

steveklabnik

mostly formatting nits

steveklabnik · 2017-01-25T06:35:53Z

src/libcore/option.rs

+    /////////////////////////////////////////////////////////////////////////
+
+    /// Inserts `v` into the option if it is None, then returns a reference to
+    /// the contained value


needs a period at the end

steveklabnik · 2017-01-25T06:36:07Z

src/libcore/option.rs

+    // Entry-like operations to insert if None and return a reference
+    /////////////////////////////////////////////////////////////////////////
+
+    /// Inserts `v` into the option if it is None, then returns a reference to


None should also be in backticks

steveklabnik · 2017-01-25T06:36:24Z

src/libcore/option.rs

+
+
+    /// Inserts a value computed from `f` into the option if it is None, then
+    /// returns a reference to the contained value


same two issues as above

steveklabnik · 2017-01-25T06:36:33Z

src/libcore/option.rs

+    }
+
+    /// Inserts `v` into the option if it is None, then
+    /// returns a mutable reference to the contained value


same as above

steveklabnik · 2017-01-25T06:36:41Z

src/libcore/option.rs

+    }
+
+    /// Inserts a value computed from `f` into the option if it is None, then
+    /// returns a mutable reference to the contained value


same as above

steveklabnik · 2017-01-25T06:37:47Z

src/libcore/option.rs

+    ///
+    /// let mut x = None;
+    /// let y: &mut u32 = x.get_mut_or_insert(5);
+    /// assert_eq!(y, &5);


could you put new lines before and after this, give it a little space?

steveklabnik · 2017-01-25T06:38:12Z

src/libcore/option.rs

+    /// let mut x = None;
+    /// let y: &mut u32 = x.get_mut_or_insert(5);
+    /// assert_eq!(y, &5);
+    /// *y = 7;


should there be another assert_eq after this line?

I didn't do it on purpose, because the only thing that was interesting here was the mutability of the returned value and I wanted the example shorter. But I am happy to add it if you think that's better?

Actually, this gave me an idea for something hopefully even better. Running tests locally before pushing new version :)

steveklabnik · 2017-01-25T06:38:22Z

src/libcore/option.rs

+    /// let mut x = None;
+    /// let y: &mut u32 = x.get_mut_or_insert_with(|| 5);
+    /// assert_eq!(y, &5);
+    /// *y = 7;


same issue as above

steveklabnik · 2017-01-25T06:38:40Z

src/libcore/option.rs

+    /// ```
+    /// #![feature(option_entry)]
+    ///
+    /// let mut x = None;


could you put a newline after this please?

steveklabnik · 2017-01-25T06:38:49Z

src/libcore/option.rs

+    /// ```
+    /// #![feature(option_entry)]
+    ///
+    /// let mut x = None;


could you put a newline after this please?

shahn · 2017-01-25T07:10:22Z

I think I addressed all your review comments, let me know if you like it. I can also squash the changes if that's preferable.

steveklabnik

two tiny more. this is great 👍 squash is probably good, but feel free to wait until someone signs off on the implementations too

steveklabnik · 2017-01-25T07:17:54Z

src/libcore/option.rs

+    ///
+    ///   *y = 7;
+    /// }
+    /// assert_eq!(x, Some(7));


one more newline above this?

this example is much better 👍

steveklabnik · 2017-01-25T07:18:03Z

src/libcore/option.rs

+    ///
+    ///   *y = 7;
+    /// }
+    /// assert_eq!(x, Some(7));


steveklabnik · 2017-01-25T07:18:29Z

src/libcore/option.rs

-    /// *y = 7;
+    ///
+    /// {
+    ///   let y: &mut u32 = x.get_mut_or_insert_with(|| 5);


steveklabnik · 2017-01-25T07:18:46Z

src/libcore/option.rs

-    /// *y = 7;
+    ///
+    /// {
+    ///   let y: &mut u32 = x.get_mut_or_insert(5);


all this code should be indented another two spaces; four is the standard style.

shahn · 2017-01-25T07:26:46Z

changes made, thanks :)

JordiPolo · 2017-01-25T09:23:59Z

I'm just learning rust but isn't this a huge api (4 new functions) for what seems like a trivial problem? Option does not have so many functions and I can guess most of the people just learn them all. This is just more to learn.

shahn · 2017-01-25T14:15:19Z

To me, this should be a part of the stdlib because for the user it's very hard to reason about the unreachable path and whether that'll be optimized out or not. Entry-style APIs should be ubiquitous, imho.

Edit: And with that comment, I think if this is deemed acceptable it should use the intrinsics unreachable for the implementation.

oli-obk · 2017-01-25T15:07:11Z

Why are the immutable versions required at all? You can always use the mutable reference like an immutable one.

shahn · 2017-01-25T15:30:19Z

Oh indeed.

oli-obk · 2017-01-25T15:35:46Z

And now you can remove the _mut, since there's no immutable alternative ^^

shahn · 2017-02-01T00:49:53Z

Ping, should I be doing anything here? Like providing a rebase or something? Hope I'm not accidentally blocking this by messing up a required step.

alexcrichton · 2017-02-04T22:16:08Z

@bors: r+

Looks good to me, thanks @shahn!

bors · 2017-02-04T22:16:09Z

📌 Commit 25c94c3 has been approved by alexcrichton

shahn · 2017-02-04T23:05:00Z

Sure I shouldn't rebase it? But yay anyway :)

alexcrichton · 2017-02-04T23:25:13Z

Nah this is still mergeable (no need for a rebase). If you want to squash down the commits though feel free and I'll re-r+

@steveklabnik

This implements rust-lang#39288. Thanks to @steveklabnik and @oli-obk for their review comments :)

shahn · 2017-02-04T23:51:15Z

I squashed it up, now less messy. Thanks

sfackler · 2017-02-05T04:25:13Z

@bors r=alexcrichton

bors · 2017-02-05T04:25:14Z

📌 Commit 8e02ad0 has been approved by alexcrichton

Provide Entry-like API for Option This implements rust-lang#39288. I am wondering whether to use std::intrinsics::unreachable!() here. Both seems fine to me (the second match optimizes away in release mode).

Rollup of 19 pull requests - Successful merges: #38518, #38921, #38959, #38983, #39009, #39107, #39193, #39289, #39312, #39393, #39442, #39443, #39453, #39454, #39471, #39477, #39478, #39527, #39552 - Failed merges:

bors · 2017-02-05T16:57:17Z

⌛ Testing commit 8e02ad0 with merge 9c8cdb2...

rust-highfive assigned brson Jan 25, 2017

steveklabnik reviewed Jan 25, 2017

View reviewed changes

steveklabnik suggested changes Jan 25, 2017

View reviewed changes

steveklabnik approved these changes Jan 25, 2017

View reviewed changes

frewsxcv added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 25, 2017

Provide Entry-like API for Option

8e02ad0

This implements rust-lang#39288. Thanks to @steveklabnik and @oli-obk for their review comments :)

shahn force-pushed the option_entry branch from 25c94c3 to 8e02ad0 Compare February 4, 2017 23:50

frewsxcv mentioned this pull request Feb 5, 2017

Rollup of 20 pull requests #39562

Closed

frewsxcv mentioned this pull request Feb 5, 2017

Rollup of 19 pull requests #39563

Merged

bors merged commit 8e02ad0 into rust-lang:master Feb 5, 2017

shahn deleted the option_entry branch February 6, 2017 04:32

tbu- mentioned this pull request Feb 6, 2017

Tracking issue: Provide Entry-like API for Option #39288

Closed



		/// Inserts a value computed from `f` into the option if it is None, then
		/// returns a reference to the contained value

Provide Entry-like API for Option #39289

Provide Entry-like API for Option #39289

Conversation

shahn commented Jan 25, 2017

rust-highfive commented Jan 25, 2017

steveklabnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahn commented Jan 25, 2017

steveklabnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahn commented Jan 25, 2017

JordiPolo commented Jan 25, 2017

shahn commented Jan 25, 2017 • edited Loading

oli-obk commented Jan 25, 2017

shahn commented Jan 25, 2017

oli-obk commented Jan 25, 2017

shahn commented Feb 1, 2017

alexcrichton commented Feb 4, 2017

bors commented Feb 4, 2017

shahn commented Feb 4, 2017

alexcrichton commented Feb 4, 2017

shahn commented Feb 4, 2017

sfackler commented Feb 5, 2017

bors commented Feb 5, 2017

bors commented Feb 5, 2017

shahn commented Jan 25, 2017 •

edited

Loading