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

Lazy mutable API extension #193

Merged
merged 23 commits into from
Sep 2, 2022
Merged

Lazy mutable API extension #193

merged 23 commits into from
Sep 2, 2022

Conversation

danielSanchezQ
Copy link
Contributor

@danielSanchezQ danielSanchezQ commented Sep 1, 2022

As requested in #113. Also added explicit get_mut.

Implement:

  • async::Lazy::force_mut
  • async::Lazy::get_mut
  • sync::Lazy::force_mut
  • sync::Lazy::get_mut

@danielSanchezQ danielSanchezQ marked this pull request as draft September 1, 2022 09:34
@danielSanchezQ
Copy link
Contributor Author

danielSanchezQ commented Sep 1, 2022

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
Copy link
Owner

matklad commented Sep 1, 2022

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

  • add some basic smoke tests for this function to it.rs (integration tests)
  • mention this change in the Changelog
  • bump the version in Cargo.toml, so that it is released as soon as it is merged

@matklad
Copy link
Owner

matklad commented Sep 1, 2022

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
Copy link
Contributor Author

danielSanchezQ commented Sep 1, 2022

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
Copy link
Contributor Author

danielSanchezQ commented Sep 1, 2022

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

@danielSanchezQ danielSanchezQ changed the title Mutable unsync::Lazy API OnceCell and Lazy mutable API extension Sep 1, 2022
@matklad
Copy link
Owner

matklad commented Sep 1, 2022

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 danielSanchezQ marked this pull request as ready for review September 1, 2022 14:21
@danielSanchezQ
Copy link
Contributor Author

danielSanchezQ commented Sep 1, 2022

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
Copy link
Owner

matklad commented Sep 1, 2022

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.

@danielSanchezQ
Copy link
Contributor Author

danielSanchezQ commented Sep 1, 2022

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
Copy link
Contributor Author

@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 😄

@danielSanchezQ danielSanchezQ changed the title OnceCell and Lazy mutable API extension Lazy mutable API extension Sep 2, 2022
CHANGELOG.md Outdated Show resolved Hide resolved
@matklad
Copy link
Owner

matklad commented Sep 2, 2022

bors r+

Thanks!

@matkad
Copy link

matkad commented Sep 2, 2022 via email

@bors
Copy link
Contributor

bors bot commented Sep 2, 2022

Build succeeded:

@bors bors bot merged commit df34da6 into matklad:master Sep 2, 2022
@matklad
Copy link
Owner

matklad commented Sep 2, 2022

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

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.

3 participants