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

refactor: replace once_cell Lazy with LazyLock #9844

Merged
merged 14 commits into from
Aug 8, 2024

Conversation

harsh-ps-2003
Copy link
Contributor

Removing the use of once_cell crate as we have LazyLock from rust 1.80 release

Fixes #9799

README.md Outdated
@@ -87,7 +87,7 @@ When updating this, also update:
- .github/workflows/lint.yml
-->

The Minimum Supported Rust Version (MSRV) of this project is [1.79.0](https://blog.rust-lang.org/2024/06/13/Rust-1.79.0.html).
The Minimum Supported Rust Version (MSRV) of this project is [1.80.0](https://blog.rust-lang.org/2024/06/13/Rust-1.80.0.html).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you need to update the blogpost link too.
@mattsse should we remove this link?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now that's updated, maybe next time :D

@DaniPopes
Copy link
Member

@harsh-ps-2003 please use nightly toolchain to format: 'cargo +nightly fmt'

README.md Outdated
@@ -87,7 +87,7 @@ When updating this, also update:
- .github/workflows/lint.yml
-->

The Minimum Supported Rust Version (MSRV) of this project is [1.79.0](https://blog.rust-lang.org/2024/06/13/Rust-1.79.0.html).
The Minimum Supported Rust Version (MSRV) of this project is [1.80.0](https://blog.rust-lang.org/2024/06/13/Rust-1.80.0.html).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now that's updated, maybe next time :D

README.md Outdated Show resolved Hide resolved
@@ -22,6 +21,7 @@ use reth_primitives_traits::{
use reth_trie_common::root::state_root_ref_unhashed;
#[cfg(feature = "std")]
use std::sync::Arc;
use std::sync::LazyLock;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file has std and no std specific elements so I think you need to do something like (should be true for other files with same cfg config)

#[cfg(feature = "std")]
use std::sync::LazyLock;

#[cfg(not(feature = "std"))]
use alloc::sync::LazyLock;

Copy link
Contributor Author

@harsh-ps-2003 harsh-ps-2003 Jul 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was troubled beacuase I thought I will have to manually implement the no-std version of std::sync::LazyLock somehow, so as to not use alloc crate at all! Thank you!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LazyLock is only available in std. It does not exist in alloc, as it depends on the OS and other system-specific stuff that are only available in std.

You must keep once_cell in the no_std case.

I would also recommend you run the command locally to see the failures instead of pushing a million commits one at a time and waiting for someone to run CI on it

@onbjerg onbjerg added the C-debt Refactor of code section that is hard to understand or maintain label Jul 29, 2024
Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs another cargo +nightly fmt, remember to update your toolchain beforehand:)

@harsh-ps-2003
Copy link
Contributor Author

The PR should be passing now! Thanks for the patience!

@Rjected Rjected requested a review from onbjerg August 2, 2024 15:44
Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, ptal @mattsse

@onbjerg onbjerg enabled auto-merge August 5, 2024 13:16
@onbjerg onbjerg changed the title Replace once_cell Lazy with LazyLock refactor: replace once_cell Lazy with LazyLock Aug 5, 2024
@onbjerg onbjerg self-assigned this Aug 5, 2024
Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, we still need to use once_cell on no_std as mentioned above. otherwise this is good to merge @harsh-ps-2003

@onbjerg onbjerg disabled auto-merge August 5, 2024 13:26
@harsh-ps-2003
Copy link
Contributor Author

I must keep once_cell in the no_std case as LazyLock is not supporting it! Can we remove the once_cell dependency completely, I dont think so? Do you think this PR should be closed, I feel we can merge it!? @onbjerg

@mattsse
Copy link
Collaborator

mattsse commented Aug 5, 2024

lets keep once cell for that crate then

@onbjerg
Copy link
Member

onbjerg commented Aug 5, 2024

Yes, but you are not actually using once_cell on no_std. You need to switch between once_cell and std's LazyLock based on whether std is available or not. See the ci failure:)

@harsh-ps-2003
Copy link
Contributor Author

Now! It should be finally done!

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool

@mattsse mattsse enabled auto-merge August 8, 2024 12:47
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pending @onbjerg

@mattsse mattsse added this pull request to the merge queue Aug 8, 2024
Merged via the queue into paradigmxyz:main with commit 106a0c7 Aug 8, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-debt Refactor of code section that is hard to understand or maintain
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace once_cell with std::sync
5 participants