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.

Sign up for GitHub

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

Jump to bottom

Replace once_cell usage with std::sync::OnceLock #1289

Merged
merged 1 commit into from
Jan 29, 2024
Merged

Replace once_cell usage with std::sync::OnceLock #1289

merged 1 commit into from
Jan 29, 2024

Conversation

andy128k
Copy link
Contributor

@andy128k andy128k commented Jan 21, 2024 •
edited
Loading

Closes #30
Closes #1141

See also gtk-rs/gir#1532 gtk-rs/gir#1537

This change does not fully remove once_cell. Full removal will break a contract of glib::Properties macro.

@andy128k andy128k mentioned this pull request Jan 21, 2024
Merged
@andy128k andy128k marked this pull request as ready for review January 21, 2024 18:26
sdroege
sdroege reviewed Jan 22, 2024
View reviewed changes
Copy link
Member

@sdroege sdroege left a comment

Choose a reason for hiding this comment

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

Looks good to me otherwise

@andy128k andy128k force-pushed the std_cells branch 2 times, most recently from bd37ee0 to e4513d2 Compare January 22, 2024 10:07
@andy128k
Copy link
Contributor Author

I guess, this PR should be merged first.

bilelmoussaoui
bilelmoussaoui reviewed Jan 25, 2024
View reviewed changes
gdk-pixbuf/src/subclass/pixbuf_animation.rs Outdated
@@ -160,8 +160,10 @@ unsafe extern "C" fn animation_get_size<T: PixbufAnimationImpl>(
}
}

static STATIC_IMAGE_QUARK: Lazy<glib::Quark> =
Lazy::new(|| glib::Quark::from_str("gtk-rs-subclass-static-image"));
fn static_image_quark() -> glib::Quark {
Copy link
Member

Choose a reason for hiding this comment

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

I am not really a fan of this change tbh, as at the end of the day we don't get rid of once_cell from our dependencies tree. I would keep such Lazy usages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May you explain? This PR literally removes usage of once_cell crate.

Copy link
Member

Choose a reason for hiding this comment

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

Never mind, I got confused by the tests/examples deps pulling once_cell.

Copy link
Member

Choose a reason for hiding this comment

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

I also don't like it much but as it's only internal it seems fine to me. We can switch to LazyLock at a later time once it's stable.

@andy128k can you create an issue about that?

Copy link
Member

Choose a reason for hiding this comment

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

I actually don't think you need a function here at all, the quark is used only inside a single function which can define the static itself. See gtk-rs/gtk4-rs@3b8e9b0

bilelmoussaoui
bilelmoussaoui previously approved these changes Jan 25, 2024
View reviewed changes
Copy link
Member

@bilelmoussaoui bilelmoussaoui left a comment

Choose a reason for hiding this comment

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

As I mentioned above, not a fan of the GQuark change but no strong opinion. Otherwise lgtm.

sdroege
sdroege previously approved these changes Jan 26, 2024
View reviewed changes
Copy link
Member

@sdroege sdroege left a comment

Choose a reason for hiding this comment

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

Looks good to me

@bilelmoussaoui bilelmoussaoui mentioned this pull request Jan 26, 2024
Merged
@andy128k andy128k force-pushed the std_cells branch 2 times, most recently from d53b865 to dbf7540 Compare January 27, 2024 10:40
bilelmoussaoui
bilelmoussaoui approved these changes Jan 27, 2024
View reviewed changes
Copy link
Member

@bilelmoussaoui bilelmoussaoui left a comment

Choose a reason for hiding this comment

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

before landing this, someone should do the same for gstreamer as they use the re-exported once_cell crate from glib iirc

@sdroege
Copy link
Member

sdroege commented Jan 29, 2024

before landing this, someone should do the same for gstreamer as they use the re-exported once_cell crate from glib iirc

But we have a Cargo.lock so I'll take care of that once this is merged :) I'll shortly go over this again and then merge it.

@sdroege sdroege merged commit a4a1688 into gtk-rs:master Jan 29, 2024
48 checks passed
@andy128k andy128k deleted the std_cells branch January 29, 2024 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdroege sdroege sdroege approved these changes

@bilelmoussaoui bilelmoussaoui bilelmoussaoui approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace once_cell with std Use once_cell::Lazy instead of std::sync::Once
3 participants
@andy128k @sdroege @bilelmoussaoui