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] - Impl AsRef+AsMut for Res, ResMut, and Mut #2189

Closed
wants to merge 1 commit into from

Conversation

anchpop
Copy link
Contributor

@anchpop anchpop commented May 16, 2021

This can save users from having to type &*X all the time at the cost of some complexity in the type signature. For instance, this allows me to accommodate @jakobhellermann's suggestion in #1799 without requiring users to type &*windows 99% of the time.

@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible labels May 16, 2021
@alice-i-cecile
Copy link
Member

Of particular note: this is likely to make sharing logic between components and resources easier. For example I would like to try to use this to create EventReaders that can be stored in both system and query parameters in #2116.

impl<'w, T: Component> AsMut<T> for ResMut<'w, T> {
fn as_mut(&mut self) -> &mut T {
&mut *self
}
}

Copy link
Member

Choose a reason for hiding this comment

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

could you put this impl next to impl<'w, T: Component> AsRef<T> for ResMut<'w, T> below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, not sure how that got mixed up

@NathanSWard
Copy link
Contributor

It may be worth marking these #[inline] perhaps?

@anchpop
Copy link
Contributor Author

anchpop commented May 17, 2021

It may be worth marking these #[inline] perhaps?

I don't mind doing it, but I'm curious - is rustc not usually aggressive enough to make that necessary?

@NathanSWard
Copy link
Contributor

I don't mind doing it, but I'm curious - is rustc not usually aggressive enough to make that necessary

Soo here's where I get a little confused, as yes I do believe the compiler is smart enough to inline this. However across the code base I've seen inline used quite liberally. I would probably defer to @bjorn3 for this.

@bjorn3
Copy link
Contributor

bjorn3 commented May 17, 2021

Because of the generics it would be possible for rustc to inline it even without #[inline], however when using -Zshare-generics this is no longer the case for things that are used by an upstream crate. As the functions are very simple it is likely faster to compile to inline them than to emit a call, so I would suggest using #[inline].

Functions can only be inlined when they are codegened as part of the same crate. This only happens for functions from upstream crates when either #[inline] is used or the function is generic and in case -Zshare-generics is used, there is no upstream compilation of the function to share.

@anchpop
Copy link
Contributor Author

anchpop commented May 17, 2021

@bjorn3 okay, done!

@NathanSWard
Copy link
Contributor

Awesome looks good to me!
As a follow-up, it would be good to change uses of & *variable to use as_ref() (and ditto with as_mut())

@NathanSWard NathanSWard 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 May 17, 2021
@cart
Copy link
Member

cart commented May 17, 2021

bors r+

bors bot pushed a commit that referenced this pull request May 17, 2021
This can save users from having to type `&*X` all the time at the cost of some complexity in the type signature. For instance, this allows me to accommodate @jakobhellermann's suggestion in #1799 without requiring users to type `&*windows` 99% of the time.
@bors bors bot changed the title Impl AsRef+AsMut for Res, ResMut, and Mut [Merged by Bors] - Impl AsRef+AsMut for Res, ResMut, and Mut May 17, 2021
@bors bors bot closed this May 17, 2021
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
This can save users from having to type `&*X` all the time at the cost of some complexity in the type signature. For instance, this allows me to accommodate @jakobhellermann's suggestion in bevyengine#1799 without requiring users to type `&*windows` 99% of the time.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Feature A new feature, making something new possible 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.

6 participants