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

Deprecating hashbrown reexports #11721

Merged
merged 1 commit into from
Feb 6, 2024
Merged

Deprecating hashbrown reexports #11721

merged 1 commit into from
Feb 6, 2024

Conversation

andristarr
Copy link
Contributor

Objective

Solution

  • Adding a deprecated tag on the re-exports, so in future releases these can be safely removed.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 5, 2024

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@mockersf
Copy link
Member

mockersf commented Feb 5, 2024

I think the idea was to deprecate just the "stable" variants

@alice-i-cecile alice-i-cecile added A-Utils Utility functions and types C-Code-Quality A section of code that is hard to understand or change labels Feb 5, 2024
alice-i-cecile
alice-i-cecile requested changes Feb 5, 2024
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Yep, we only want to deprecate the stable variants.

We should also change the note to just explain exactly what this was a type alias was for: just copy the type from the source code :)

@andristarr
Copy link
Contributor Author

andristarr commented Feb 5, 2024 •
edited
Loading

Sorry, why only the stable ones: as in, the other 2 seem to only be also just a reexport without any added functionality

Edit: nevermind, I think I see why.

@alice-i-cecile
Copy link
Member

Yeah, we use the faster ones internally all over the place, and they're a good default for games.

@andristarr andristarr force-pushed the deprecate-StableHashMap-and-StableHashSet branch from c5ae6d4 to 4f66e92 Compare February 5, 2024 16:42
@andristarr
Copy link
Contributor Author

andristarr commented Feb 5, 2024 •
edited
Loading

Let me know if the wording for the deprecated note look good from your side.

@andristarr andristarr force-pushed the deprecate-StableHashMap-and-StableHashSet branch from 4f66e92 to 83257ac Compare February 5, 2024 16:53
alice-i-cecile
alice-i-cecile requested changes Feb 5, 2024
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Deprecation notice looks good :)

Hmm. @SpecificProtagonist, looks like we do actually use this internally.

We should migrate off the internal code as part of this PR if we want to pursue this. But given that we do use it internally, I'm weakly in favor of just leaving it alone and closing this issue and PR. @andristarr thanks for your work here either way; this was good to discover.

@SpecificProtagonist
Copy link
Contributor

SpecificProtagonist commented Feb 5, 2024 •
edited
Loading

Hmm. @SpecificProtagonist, looks like we do actually use this internally.

Where is that? The use in GenericTypeCell wasn't for stability, it was only for const constructibility, and isn't in main anymore.

@alice-i-cecile
Copy link
Member

Hmm okay. @andristarr can you rebase this PR to latest main for us?

doonv
doonv suggested changes Feb 6, 2024
View reviewed changes
crates/bevy_utils/src/lib.rs Outdated
Comment on lines 117 to 128
#[deprecated(
note = "Will be required to use the hash library of your choice. Alias for: hashbrown::HashMap<K, V, FixedState>"
)]
pub type StableHashSet<K> = hashbrown::HashSet<K, FixedState>;
Copy link
Contributor

Choose a reason for hiding this comment

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

This note is incorrect (It's copy pasted from the StableHashMap one)

@andristarr andristarr force-pushed the deprecate-StableHashMap-and-StableHashSet branch from 83257ac to 6ecabf7 Compare February 6, 2024 17:11
@andristarr andristarr force-pushed the deprecate-StableHashMap-and-StableHashSet branch from 6ecabf7 to 49e0089 Compare February 6, 2024 17:15
@andristarr
Copy link
Contributor Author

Done, should be ready to go!

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 6, 2024
@alice-i-cecile
Copy link
Member

Looks great, thanks for the prompt and friendly responses to the reviews :)

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 6, 2024
Merged via the queue into bevyengine:main with commit 9f2eabb Feb 6, 2024
24 checks passed
@tguichaoua tguichaoua mentioned this pull request Feb 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@doonv doonv doonv requested changes

@NthTensor NthTensor NthTensor approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

Assignees
No one assigned
Labels
A-Utils Utility functions and types C-Code-Quality A section of code that is hard to understand or change S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deprecate StableHashMap/StableHashSet
6 participants
@andristarr @mockersf @alice-i-cecile @SpecificProtagonist @NthTensor @doonv