Add borrow_with to LazyCell #56

archer884 · 2016-12-07T03:04:54Z

Pursuant to some commentary from https://www.reddit.com/r/rust/comments/5gv3k9/lazy_fields, this adds .borrow_with() to LazyCell only. I tinkered with adding it to the atomic version, but it seemed to me the api would not be as clean there and, as a result, not nearly as useful.

coveralls · 2016-12-07T04:35:19Z

Coverage decreased (-20.5%) to 79.512% when pulling 5a34cdc on archer884:or-insert-with into ed58634 on indiv0:master.

archer884 · 2016-12-07T08:08:35Z

Also, sorry, I really meant to open this pr against my fork, because I actually have not gotten the chance to go over the contributing.md yet. :(

coveralls · 2016-12-07T08:52:33Z

Coverage decreased (-21.5%) to 78.537% when pulling 0540b51 on archer884:or-insert-with into ed58634 on indiv0:master.

indiv0 · 2016-12-07T09:37:03Z

No worries :)

Once you've read through the CONTRIBUTING.md and got the PR up to standard, just ping me and I'll review it.

archer884 · 2016-12-07T09:39:59Z

@indiv0 Actually I think it's good now... Looked like I just needed to fix the commit message? Be happy to fix anything else that comes up.

indiv0 · 2016-12-07T09:51:02Z

src/lib.rs

+    /// If the underlying value of the cell has not yet been realized, the
+    /// cell is first filled using the function provided.
+    pub fn borrow_with<F: FnOnce() -> T>(&self, f: F) -> &T {
+        let mut slot = unsafe {&mut *self.inner.get() };


Please add a space before &mut (i.e. { &mut). Picky, I know.

No, not picky at all--that's an embarrassing oversight on my part. :p

indiv0

Sorry for the very nitpicky review. Overall I agree it and I think it'll be a good addition to the API.

indiv0 · 2016-12-07T09:51:43Z

src/lib.rs

@@ -85,6 +85,21 @@ impl<T> LazyCell<T> {
        unsafe { &*self.inner.get() }.as_ref()
    }

+    /// Borrows the contents of this lazy cell for the duration of the cell
+    /// itself


Please add a period at the end of the sentence.

indiv0 · 2016-12-07T09:53:38Z

src/lib.rs

+    /// Borrows the contents of this lazy cell for the duration of the cell
+    /// itself
+    /// 
+    /// If the underlying value of the cell has not yet been realized, the


Could you replace this sentence with: "If the cell has not yet been filled, the cell is first filled using the function provided"

indiv0 · 2016-12-07T09:54:31Z

src/lib.rs

+        let lazycell = LazyCell::new();
+
+        let value = lazycell.borrow_with(|| 1);
+        assert_eq!(&1, value);


Could you also test the positive case (i.e. borrow_with(|| 1) on a cell that's already been filled (just for complete test coverage)).

Yes, I'll make a note to write that up. I was also thinking a test to assert that the || filler_func only gets called once would be appropriate.

indiv0 · 2016-12-07T09:55:48Z

src/lib.rs

+        }
+
+        *slot = Some(f());
+        slot.as_ref().unwrap()


Could you rewrite the function to be:

... if !slot.is_some() { *slot = Some(f()); } slot.as_ref().unwrap()

archer884 · 2016-12-07T09:56:55Z

@indiv0 Do you have any idea why coveralls is complaining about this? I guess I may not understand how the coverage test is intended to work, but it seems weird that it would go down so much--seems like the new code would be covered? oO

indiv0 · 2016-12-07T09:58:34Z

Coveralls is complaining so much (I believe) because kcov changed how they handle Rust code a while back, so it was artificially inflated before. I haven't modified the codebase since then, so that's why the drop occurs in your PR. I think if you add the "positive case" I mentioned above, it'll push the PR over the 80% minimum again.

Also, don't forget to add yourself to the CONTRIBUTORS.md!

coveralls · 2016-12-07T10:02:20Z

Coverage decreased (-20.0%) to 80.0% when pulling 538226d on archer884:or-insert-with into ed58634 on indiv0:master.

Add a `borrow_with(|| foo)` convenience method to `LazyCell` so that users may provide a function to automatically fill the cell in the event that it is empty on attempting to borrow.

archer884 · 2016-12-07T18:36:25Z

@indiv0 Looks like you were right about that coverage--and I think I now have a better understanding of what it's measuring. Don't worry about the review! Not nitpicky at all. :)

Was just thinking that I did not update the version in Cargo.toml and that's not in the contributing doc, either--is that something that clog is going to do? If so, that's kinda neat.

coveralls · 2016-12-07T18:47:50Z

Coverage decreased (-19.6%) to 80.357% when pulling 8af5eb9 on archer884:or-insert-with into ed58634 on indiv0:master.

coveralls · 2016-12-07T19:34:37Z

Coverage decreased (-23.3%) to 76.682% when pulling a15efa3 on archer884:or-insert-with into ed58634 on indiv0:master.

indiv0 · 2016-12-08T04:33:02Z

sigh coveralls is still being weird. But that's fine. Updating the version and making a release must be done manually, unfortunately, but that's something I'll handle on my end, right now.

Thank you very much for this PR!

indiv0 · 2016-12-08T04:56:24Z

Actually I think I figured out the source of the remaining weirdness. Coveralls is being run for both Linux and OS X builds, and they're giving differing results. I'll probably disable coveralls on one to avoid this.

indiv0 · 2016-12-08T04:59:30Z

Either way, v0.5.0 is published. Thanks again!

https://github.com/indiv0/lazycell/releases/tag/v0.5.0
https://crates.io/crates/lazycell/0.5.0

archer884 force-pushed the or-insert-with branch from 5a34cdc to 0540b51 Compare December 7, 2016 08:43

indiv0 reviewed Dec 7, 2016

View reviewed changes

archer884 force-pushed the or-insert-with branch from 0540b51 to 538226d Compare December 7, 2016 09:54

indiv0 requested changes Dec 7, 2016

View reviewed changes

archer884 force-pushed the or-insert-with branch from 538226d to 8af5eb9 Compare December 7, 2016 18:07

feat: add borrow_with to LazyCell

a15efa3

Add a `borrow_with(|| foo)` convenience method to `LazyCell` so that users may provide a function to automatically fill the cell in the event that it is empty on attempting to borrow.

archer884 force-pushed the or-insert-with branch from 8af5eb9 to a15efa3 Compare December 7, 2016 18:20

indiv0 merged commit 177a715 into indiv0:master Dec 8, 2016

archer884 deleted the or-insert-with branch December 8, 2016 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add borrow_with to LazyCell #56

Add borrow_with to LazyCell #56

archer884 commented Dec 7, 2016 •

edited

Loading

coveralls commented Dec 7, 2016 •

edited

Loading

archer884 commented Dec 7, 2016

coveralls commented Dec 7, 2016 •

edited

Loading

indiv0 commented Dec 7, 2016

archer884 commented Dec 7, 2016

indiv0 Dec 7, 2016

archer884 Dec 7, 2016

indiv0 left a comment

indiv0 Dec 7, 2016

indiv0 Dec 7, 2016

indiv0 Dec 7, 2016 •

edited

Loading

archer884 Dec 7, 2016

indiv0 Dec 7, 2016

archer884 commented Dec 7, 2016

indiv0 commented Dec 7, 2016

coveralls commented Dec 7, 2016 •

edited

Loading

archer884 commented Dec 7, 2016 •

edited

Loading

coveralls commented Dec 7, 2016 •

edited

Loading

coveralls commented Dec 7, 2016 •

edited

Loading

indiv0 commented Dec 8, 2016

indiv0 commented Dec 8, 2016 •

edited

Loading

indiv0 commented Dec 8, 2016

Add borrow_with to LazyCell #56

Add borrow_with to LazyCell #56

Conversation

archer884 commented Dec 7, 2016 • edited Loading

coveralls commented Dec 7, 2016 • edited Loading

archer884 commented Dec 7, 2016

coveralls commented Dec 7, 2016 • edited Loading

indiv0 commented Dec 7, 2016

archer884 commented Dec 7, 2016

indiv0 Dec 7, 2016

Choose a reason for hiding this comment

archer884 Dec 7, 2016

Choose a reason for hiding this comment

indiv0 left a comment

Choose a reason for hiding this comment

indiv0 Dec 7, 2016

Choose a reason for hiding this comment

indiv0 Dec 7, 2016

Choose a reason for hiding this comment

indiv0 Dec 7, 2016 • edited Loading

Choose a reason for hiding this comment

archer884 Dec 7, 2016

Choose a reason for hiding this comment

indiv0 Dec 7, 2016

Choose a reason for hiding this comment

archer884 commented Dec 7, 2016

indiv0 commented Dec 7, 2016

coveralls commented Dec 7, 2016 • edited Loading

archer884 commented Dec 7, 2016 • edited Loading

coveralls commented Dec 7, 2016 • edited Loading

coveralls commented Dec 7, 2016 • edited Loading

indiv0 commented Dec 8, 2016

indiv0 commented Dec 8, 2016 • edited Loading

indiv0 commented Dec 8, 2016

archer884 commented Dec 7, 2016 •

edited

Loading

coveralls commented Dec 7, 2016 •

edited

Loading

coveralls commented Dec 7, 2016 •

edited

Loading

indiv0 Dec 7, 2016 •

edited

Loading

coveralls commented Dec 7, 2016 •

edited

Loading

archer884 commented Dec 7, 2016 •

edited

Loading

coveralls commented Dec 7, 2016 •

edited

Loading

coveralls commented Dec 7, 2016 •

edited

Loading

indiv0 commented Dec 8, 2016 •

edited

Loading