matklad / once_cell

Rust library for single assignment cells and lazy statics without macros
Apache License 2.0
1.84k stars 110 forks source link

`Lazy` mutable API extension #193

Closed danielSanchezQ closed 2 years ago

danielSanchezQ commented 2 years ago

As requested in #113. Also added explicit get_mut.

Implement:

danielSanchezQ commented 2 years ago

It do not really makes sense to add this API to sync righ @matklad ? I also don't know if there is need of more documentation/examples for this. If so, point me to it please ๐Ÿ˜ƒ And, if there is anything that would relate to this I would be happy to give it a try too.

matklad commented 2 years ago

This basically looks good to me! Thigs left to do:

matklad commented 2 years ago

It do not really makes sense to add this API to sync righ @matklad ?

oups, missed this Q, sorry.

No, I think it does make sense to have this for sync as well, the same as sync::OnceCell::get_mut

danielSanchezQ commented 2 years ago

No, I think it does make sense to have this for sync as well, the same as sync::OnceCell::get_mut

Cool, will take a look at how sync::OnceCell::get_mut is implemented and add it.

danielSanchezQ commented 2 years ago

@matklad I added same mut API for OnceCell, which is then used for sync::Lazy.

matklad commented 2 years ago

I feel that in the latest iteration it maybe scope-crept to far. I don't think we need to extend OnceCell APIs -- just get_mut().unwrap() should be enough. If the call-site really cares about not having a panic branch there, it can use .unwrap_unchecked.

danielSanchezQ commented 2 years ago

I feel that in the latest iteration it maybe scope-crept to far. I don't think we need to extend OnceCell APIs -- just get_mut().unwrap() should be enough. If the call-site really cares about not having a panic branch there, it can use .unwrap_unchecked.

@matkad, you mean all the implementations or just the get_mut_unchecked? I agree that get_mut_unchecked may be too much, but I added it as I wanted to replicate the API. But totally down to remove it again ๐Ÿ˜„

matklad commented 2 years ago

I think we only need to add these four APIs

And OnceCell::get_mut should be enough to implement those.

danielSanchezQ commented 2 years ago

I think we only need to add these four APIs

  • unsync::Lazy::force_mut
  • unsync::Lazy::get_mut
  • sync::Lazy::force_mut
  • sync::Lazy::get_mut

And OnceCell::get_mut should be enough to implement those.

Not to discuss here probably. But having the option to get the &mut as well as the & when getting or initializing end up solving the issue in where you end up with something like:

let cell = OnceCell::new();
cell.get_or_init(|| String::new("foo"));
let mut mut_ref = cell.get_mut().unwrap();

and instead keep the api as expected:

let cell = OnceCell::new();
let mut mut_ref = cell.get_mut_or_init(|| String::new("foo"));

It seems not too much, but bump into that already in the past (and today once again ๐Ÿ˜… ).

danielSanchezQ commented 2 years ago

@matklad , I left just the Lazy items as discussed. I will open an issue+PR with the OnceCell ones so we do not mix discussions ๐Ÿ˜„

matklad commented 2 years ago

bors r+

Thanks!

matkad commented 2 years ago

Hi,

donโ€™t really know how my email got onto this distribution list :-) being a programmer myself, I keep fingers crossed for the success of your project. Let me know when itโ€™s done, but please I donโ€™t need to see all the small updates :D

Cheers, Mateusz

On 2 Sep 2022, at 13:48, Aleksey Kladov @.***> wrote:

bors r+

Thanks!

โ€” Reply to this email directly, view it on GitHub https://github.com/matklad/once_cell/pull/193#issuecomment-1235401938, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGDPX3GK7EAYFCJO5QVTM3TV4HSQ5ANCNFSM6AAAAAAQCFKW6M. You are receiving this because you were mentioned.

bors[bot] commented 2 years ago

Build succeeded:

matklad commented 2 years ago

matkad sorry, I think that was a typo in a mention due to our github logins having a hamming distance of one: https://github.com/matkad vs https://github.com/matklad