Cleanup unneeded lifetimes in bevy_asset#15546
Merged
alice-i-cecile merged 1 commit intobevyengine:mainfrom Sep 30, 2024
Merged
Cleanup unneeded lifetimes in bevy_asset#15546alice-i-cecile merged 1 commit intobevyengine:mainfrom
alice-i-cecile merged 1 commit intobevyengine:mainfrom
Conversation
d0a3521 to
8cc475f
Compare
alice-i-cecile
approved these changes
Sep 30, 2024
Member
|
I love these changes: I'm largely going to let you fight CI here and let you two hash out what can and can't be done ;) |
da89e3f to
a54b6ac
Compare
a54b6ac to
2b61a99
Compare
Contributor
Author
I'm done fighting now, there was some give and take so let's call it a draw. |
aecsocket
reviewed
Sep 30, 2024
Contributor
aecsocket
left a comment
There was a problem hiding this comment.
Nice and straightforward, I found a few more places where you can get rid of lifetimes
tim-blackbird
approved these changes
Sep 30, 2024
aecsocket
approved these changes
Sep 30, 2024
robtfm
pushed a commit
to robtfm/bevy
that referenced
this pull request
Oct 4, 2024
# Objective Fixes bevyengine#15541 A bunch of lifetimes were added during the Assets V2 rework, but after moving to async traits in bevyengine#12550 they can be elided. That PR mentions that this might be the case, but apparently it wasn't followed up on at the time. ~~I ended up grepping for `<'a` and finding a similar case in `bevy_reflect` which I also fixed.~~ (edit: that one was needed apparently) Note that elided lifetimes are unstable in `impl Trait`. If that gets stabilized then we can elide even more. ## Solution Remove the extra lifetimes. ## Testing Everything still compiles. If I have messed something up there is a small risk that some user code stops compiling, but all the examples still work at least. --- ## Migration Guide The traits `AssetLoader`, `AssetSaver` and `Process` traits from `bevy_asset` now use elided lifetimes. If you implement these then remove the named lifetime.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Objective
Fixes #15541
A bunch of lifetimes were added during the Assets V2 rework, but after moving to async traits in #12550 they can be elided. That PR mentions that this might be the case, but apparently it wasn't followed up on at the time.
I ended up grepping for(edit: that one was needed apparently)<'aand finding a similar case inbevy_reflectwhich I also fixed.Note that elided lifetimes are unstable in
impl Trait. If that gets stabilized then we can elide even more.Solution
Remove the extra lifetimes.
Testing
Everything still compiles. If I have messed something up there is a small risk that some user code stops compiling, but all the examples still work at least.
Migration Guide
The traits
AssetLoader,AssetSaverandProcesstraits frombevy_assetnow use elided lifetimes. If you implement these then remove the named lifetime.