Skip to content

[Merged by Bors] - Fix all_tuples macro for non-0/1 starts #4002

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
Closed
wants to merge 1 commit into from
Closed

[Merged by Bors] - Fix all_tuples macro for non-0/1 starts #4002

wants to merge 1 commit into from

Conversation

@james7132
Copy link
Member

@james7132 james7132 commented Feb 21, 2022

Objective

all_tuples panics when the start count is set to anything other than 0 or 1. Fix this bug.

Solution

Originally part of #2381, this PR fixes the slice indexing used by the proc macro.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Feb 21, 2022
@james7132 james7132 added A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change and removed S-Needs-Triage This issue needs to be labelled labels Feb 21, 2022
DJMcNab
DJMcNab approved these changes Feb 21, 2022
View reviewed changes
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

The change in itself is fine, although the motivation is unclear. I don't think we want all_tuples to be part of our public API, and obviously no internal uses run into this case.

@james7132
Copy link
Member Author

james7132 commented Feb 21, 2022 •
edited
Loading

Ran into it as a part of #2381, where it was explicitly used in a case where the start for the macro was set at 2 to avoid confusing duplicate cases in the API.

This has since been made into an ecosystem crate (bevy_system_graph) instead of being merged into bevy_ecs, which is now as an external consumer of the macro.

@DJMcNab
Copy link
Member

DJMcNab commented Feb 21, 2022

I just saw that all_tuples is re-exported at the root of bevy_ecs. I agree then that using it publicly is 'fair'. However, I don't think we should be exporting it; evidence in favour of that is that it has no docs.

Since it is useful outside of our internal use, all_tuples should probably be spun out into its own crate.

@mockersf
Copy link
Member

mockersf commented Feb 21, 2022

I just saw that all_tuples is re-exported at the root of bevy_ecs. I agree then that using it publicly is 'fair'. However, I don't think we should be exporting it; evidence in favour of that is that it has no docs.

The macro is used in bevy_render so it has to be public...

@cart
Copy link
Member

cart commented Feb 21, 2022

bors r+

@cart
Copy link
Member

cart commented Feb 21, 2022

My preference is that we leave this public and move it to bevy_utils. I don't think it deserves its own crate.

bors bot pushed a commit that referenced this pull request Feb 21, 2022
@james7132
Fix all_tuples macro for non-0/1 starts (#4002)
5afda8d 
# Objective
`all_tuples` panics when the start count is set to anything other than 0 or 1. Fix this bug.

## Solution
Originally part of #2381, this PR fixes the slice indexing used by the proc macro.
@bors
Copy link
Contributor

bors bot commented Feb 22, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Fix all_tuples macro for non-0/1 starts [Merged by Bors] - Fix all_tuples macro for non-0/1 starts Feb 22, 2022
@bors bors bot closed this Feb 22, 2022
kurtkuehnert pushed a commit to kurtkuehnert/bevy that referenced this pull request Mar 6, 2022
@james7132
Fix all_tuples macro for non-0/1 starts (bevyengine#4002)
376f2c6 
# Objective
`all_tuples` panics when the start count is set to anything other than 0 or 1. Fix this bug.

## Solution
Originally part of bevyengine#2381, this PR fixes the slice indexing used by the proc macro.
@james7132 james7132 deleted the all-tuples-fix branch March 21, 2022 01:37
@james7132 james7132 restored the all-tuples-fix branch June 22, 2022 08:26
@james7132 james7132 deleted the all-tuples-fix branch November 18, 2022 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

+1 more reviewer

@DJMcNab DJMcNab DJMcNab approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@james7132 @DJMcNab @mockersf @cart @alice-i-cecile