Skip to content

refactor: replace OnceLock with LazyLock #13641

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

Merged
merged 2 commits into from
Dec 4, 2024
Merged

Conversation

jonahgao
Copy link
Member

@jonahgao jonahgao commented Dec 4, 2024

Which issue does this PR close?

Part of #11687

Clear the TODOs after updating MSRV to 1.80.

Rationale for this change

LazyLock has been stable since Rust version 1.80.0, and it provides a simpler and more suitable way to define static variables.

What changes are included in this PR?

Are these changes tested?

Yes. By existing tests.

Are there any user-facing changes?

No

@github-actions github-actions bot added logical-expr Logical plan and expressions common Related to common crate labels Dec 4, 2024
@jonahgao jonahgao marked this pull request as draft December 4, 2024 08:55
@jonahgao jonahgao marked this pull request as ready for review December 4, 2024 09:00
// TODO: Use LazyLock instead of getter function when MSRV gets bumped
static $name: OnceLock<LogicalTypeRef> = OnceLock::new();
static $name: LazyLock<LogicalTypeRef> =
LazyLock::new(|| Arc::new(NativeType::$ty));

#[doc = "Getter for singleton instance of a logical type representing"]
#[doc = concat!("[`NativeType::", stringify!($ty), "`].")]
pub fn $getter() -> LogicalTypeRef {
Copy link
Member Author

Choose a reason for hiding this comment

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

Current callers require a LogicalTypeRef, so this getter still seems to be needed.

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks @jonahgao

THere appear to be quite a few more uses of OnceLock in th code too:

https://github.com/search?q=repo%3Aapache%2Fdatafusion+OnceLock&type=code

Do you plan on moving them too?

I also linked this PR to #11687 which has a few other PRs linked as well that were in draft

@comphead
Copy link
Contributor

comphead commented Dec 4, 2024

Thats good point, wondering we probably want to use LazyLock in documentation generation

@comphead comphead merged commit 9fbd87f into apache:main Dec 4, 2024
26 checks passed
@jonahgao jonahgao deleted the lazy_lock branch December 5, 2024 01:54
@jonahgao
Copy link
Member Author

jonahgao commented Dec 5, 2024

Thanks @alamb @comphead . I plan to handle the remaining OnceLocks.

zhuliquan pushed a commit to zhuliquan/datafusion that referenced this pull request Dec 6, 2024
* refactor: Replace OnceLock with LazyLock

* Fix typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Related to common crate logical-expr Logical plan and expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants