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

[Merged by Bors] - Move to smallvec v1.6 #2074

Closed
wants to merge 2 commits into from

Conversation

TheRawMeatball
Copy link
Member

No description provided.

@TheRawMeatball
Copy link
Member Author

Since we're already using latest stable, should be enable the union and const_generics features?

@alice-i-cecile alice-i-cecile added dependencies A-ECS Entities, components, systems, and events labels May 1, 2021
@TheRawMeatball TheRawMeatball added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed A-ECS Entities, components, systems, and events labels May 2, 2021
@mockersf
Copy link
Member

mockersf commented May 2, 2021

yes for union!

no for const_generics: https://docs.rs/smallvec/1.6.1/smallvec/#const_generics, unstable / needs nightly according to their doc

@TheRawMeatball
Copy link
Member Author

TheRawMeatball commented May 3, 2021

The docs aren't up-to-date: in 1.6, it compiles on stable :)

servo/rust-smallvec@8419e95

@cart
Copy link
Member

cart commented May 3, 2021

Ooh yeah lets enables those features :)

@TheRawMeatball
Copy link
Member Author

Hmm, what would be a good place to enable these features?

I'm adding a smallvec dependency to bevy_ecs in #2071, maybe that's a good place to add it? Or should we add it in all places that have a dependency already?

@cart
Copy link
Member

cart commented May 4, 2021

Hmm I think it doesn't matter too much as features are merged across dependencies. Keeping them in sync makes it easier to do a simple "search for smallvec = globally and ensure everything lines up", but it also means a crate is opting into features it might not need.

Does smallvec respect the soft "features are additive only" rule or will enabling something like const_generics break apis that weren't coded to it?

We could also consider re-exporting smallvec from bevy_utils, but I'm not sure I want that to become a dumping ground for every shared dependency with custom features.

@TheRawMeatball
Copy link
Member Author

Does smallvec respect the soft "features are additive only" rule or will enabling something like const_generics break apis that weren't coded to it?

From what I can see, union is an impl-level optimization that shouldn't break anything, and enabling const generics simply adds more impls.

@mockersf
Copy link
Member

mockersf commented May 4, 2021

We could also consider re-exporting smallvec from bevy_utils, but I'm not sure I want that to become a dumping ground for every shared dependency with custom features.

I don't think that's necessary, those features will be enabled if someone import smallvec and bevy in their project even if they don't enable them themselves

@cart
Copy link
Member

cart commented May 5, 2021

bors r+

bors bot pushed a commit that referenced this pull request May 5, 2021
@bors bors bot changed the title Move to smallvec v1.6 [Merged by Bors] - Move to smallvec v1.6 May 5, 2021
@bors bors bot closed this May 5, 2021
Copy link

@y15un y15un left a comment

Choose a reason for hiding this comment

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

hi, i found these two instances of simple typo; cargo was complaining while I was trying to run examples.

smallvec = "1.4.2"
# TODO: replace once_cell with std equivalent if/when this lands: https://github.com/rust-lang/rfcs/pull/2788
once_cell = "1.4.1"
smallvec = { version = "1.6", feautres = ["union", "const_generics"] }
Copy link

Choose a reason for hiding this comment

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

typo found: feautres => features.

cargo is complaining about this "unused manifest key."

smallvec = { version = "1.6", feautres = ["union", "const_generics"] }
Copy link

Choose a reason for hiding this comment

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

typo found: feautres => features.

cargo is complaining about this "unused manifest key."

@mockersf
Copy link
Member

mockersf commented May 5, 2021

hi, i found these two instances of simple typo; cargo was complaining while I was trying to run examples.

they could be fixed in #2111

ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

5 participants