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

[Merged by Bors] - Added missing details to SystemParam Local documentation. #7106

Closed
wants to merge 2 commits into from
Closed

[Merged by Bors] - Added missing details to SystemParam Local documentation. #7106

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
40ea646
Added missing details to SystemParam Local documentation. Renamed Loc…
iiYese Jan 6, 2023
b203449
Update crates/bevy_ecs/src/system/system_param.rs
iiYese Jan 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions crates/bevy_ecs/src/system/system_param.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -798,6 +798,10 @@ unsafe impl SystemParamState for WorldState {
///
/// A local may only be accessed by the system itself and is therefore not visible to other systems.
/// If two or more systems specify the same local type each will have their own unique local.
/// If multiple [`SystemParam`]s within the same system each specify the same local type
/// each will get their own distinct data storage.
///
/// The supplied lifetime parameter is the [`SystemParam`]s `'s` lifetime.
Copy link
Member

Choose a reason for hiding this comment

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

IMO this comment is unnecessary, simply renaming the lifetime to 's is good enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While just having 's does express the information it expresses it in a manner that is easy to miss. And given the cost of missing that information

Users can get complicated incorrect suggested fixes if they pass the wrong lifetime

I think it is worth repeating once in a more explicit visible fashion.

Copy link
Member

@JoJoJet JoJoJet Jan 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Still not convinced that it's necessary, since that comment isn't adding any information that you can't gather just from glancing at the type.

This isn't a significant complaint though, so I won't try and block on it or anything.

///
/// # Examples
///
Expand Down Expand Up @@ -837,12 +841,12 @@ unsafe impl SystemParamState for WorldState {
/// // .add_system(reset_to_system(my_config))
/// # assert_is_system(reset_to_system(Config(10)));
/// ```
pub struct Local<'a, T: FromWorld + Send + 'static>(pub(crate) &'a mut T);
pub struct Local<'s, T: FromWorld + Send + 'static>(pub(crate) &'s mut T);

// SAFETY: Local only accesses internal state
unsafe impl<'a, T: FromWorld + Send + 'static> ReadOnlySystemParam for Local<'a, T> {}
unsafe impl<'s, T: FromWorld + Send + 'static> ReadOnlySystemParam for Local<'s, T> {}

impl<'a, T: FromWorld + Send + Sync + 'static> Debug for Local<'a, T>
impl<'s, T: FromWorld + Send + Sync + 'static> Debug for Local<'s, T>
where
T: Debug,
{
Expand All @@ -851,7 +855,7 @@ where
}
}

impl<'a, T: FromWorld + Send + Sync + 'static> Deref for Local<'a, T> {
impl<'s, T: FromWorld + Send + Sync + 'static> Deref for Local<'s, T> {
type Target = T;

#[inline]
Expand All @@ -860,14 +864,14 @@ impl<'a, T: FromWorld + Send + Sync + 'static> Deref for Local<'a, T> {
}
}

impl<'a, T: FromWorld + Send + Sync + 'static> DerefMut for Local<'a, T> {
impl<'s, T: FromWorld + Send + Sync + 'static> DerefMut for Local<'s, T> {
#[inline]
fn deref_mut(&mut self) -> &mut Self::Target {
self.0
}
}

impl<'w, 'a, T: FromWorld + Send + 'static> IntoIterator for &'a Local<'w, T>
impl<'s, 'a, T: FromWorld + Send + 'static> IntoIterator for &'a Local<'s, T>
where
&'a T: IntoIterator,
{
Expand All @@ -879,7 +883,7 @@ where
}
}

impl<'w, 'a, T: FromWorld + Send + 'static> IntoIterator for &'a mut Local<'w, T>
impl<'s, 'a, T: FromWorld + Send + 'static> IntoIterator for &'a mut Local<'s, T>
where
&'a mut T: IntoIterator,
{
Expand All @@ -895,7 +899,7 @@ where
#[doc(hidden)]
pub struct LocalState<T: Send + 'static>(pub(crate) SyncCell<T>);

impl<'a, T: FromWorld + Send + 'static> SystemParam for Local<'a, T> {
impl<'s, T: FromWorld + Send + 'static> SystemParam for Local<'s, T> {
type State = LocalState<T>;
}

Expand Down