[Merged by Bors] - Newtype ArchetypeRow and TableRow#4878
Closed
james7132 wants to merge 32 commits intobevyengine:mainfrom
Closed
[Merged by Bors] - Newtype ArchetypeRow and TableRow#4878james7132 wants to merge 32 commits intobevyengine:mainfrom
james7132 wants to merge 32 commits intobevyengine:mainfrom
Conversation
alice-i-cecile
added
A-ECS
james7132
added
the
M-Migration-Guide
alice-i-cecile
approved these changes
May 31, 2022
Member
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
Changes and motivation look good. Approval conditional on non-regressing benchmarks :)
Member
Author
|
A quick round of benchmarks seems to show regressions in dense iteration with a significant speedup to Details |
james7132
added
S-Blocked
james7132
removed
the
S-Blocked
Member
|
@james7132 wanna try another round of benchmarks? |
alice-i-cecile
added
the
S-Needs-Benchmarking
Member
Author
|
Done with a set of microbenchmarks relative to the current main. Most of the prior regressions have become slight improvements, or are within the noise threshold at this point. However, there are a few outstanding regressions that have gotten noticeably larger. I'll see if I can smooth these out. Details |
BoxyUwU
suggested changes
Dec 4, 2022
BoxyUwU
approved these changes
Dec 5, 2022
BoxyUwU
added
the
S-Ready-For-Final-Review
james7132
changed the title
Member
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Dec 5, 2022
# Objective Prevent future unsoundness that was seen in #6623. ## Solution Newtype both indexes in `Archetype` and `Table` as `ArchetypeRow` and `TableRow`. This avoids weird numerical manipulation on the indices, and can be stored and treated opaquely. Also enforces the source and destination of where these indices at a type level. --- ## Changelog Changed: `Archetype` indices and `Table` rows have been newtyped as `ArchetypeRow` and `TableRow`.
Contributor
|
Build failed: |
Member
|
No merge conflicts, but merging #6843 broke this build. |
Member
Author
|
Had to add |
BoxyUwU
approved these changes
Dec 6, 2022
Member
Author
|
bors retry |
bors bot
pushed a commit
that referenced
this pull request
Dec 6, 2022
# Objective Prevent future unsoundness that was seen in #6623. ## Solution Newtype both indexes in `Archetype` and `Table` as `ArchetypeRow` and `TableRow`. This avoids weird numerical manipulation on the indices, and can be stored and treated opaquely. Also enforces the source and destination of where these indices at a type level. --- ## Changelog Changed: `Archetype` indices and `Table` rows have been newtyped as `ArchetypeRow` and `TableRow`.
Contributor
bors
bot
changed the title
ickshonpe
pushed a commit
to ickshonpe/bevy
that referenced
this pull request
Dec 6, 2022
# Objective Prevent future unsoundness that was seen in bevyengine#6623. ## Solution Newtype both indexes in `Archetype` and `Table` as `ArchetypeRow` and `TableRow`. This avoids weird numerical manipulation on the indices, and can be stored and treated opaquely. Also enforces the source and destination of where these indices at a type level. --- ## Changelog Changed: `Archetype` indices and `Table` rows have been newtyped as `ArchetypeRow` and `TableRow`.
alradish
pushed a commit
to alradish/bevy
that referenced
this pull request
Jan 22, 2023
# Objective Prevent future unsoundness that was seen in bevyengine#6623. ## Solution Newtype both indexes in `Archetype` and `Table` as `ArchetypeRow` and `TableRow`. This avoids weird numerical manipulation on the indices, and can be stored and treated opaquely. Also enforces the source and destination of where these indices at a type level. --- ## Changelog Changed: `Archetype` indices and `Table` rows have been newtyped as `ArchetypeRow` and `TableRow`.
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
# Objective Prevent future unsoundness that was seen in bevyengine#6623. ## Solution Newtype both indexes in `Archetype` and `Table` as `ArchetypeRow` and `TableRow`. This avoids weird numerical manipulation on the indices, and can be stored and treated opaquely. Also enforces the source and destination of where these indices at a type level. --- ## Changelog Changed: `Archetype` indices and `Table` rows have been newtyped as `ArchetypeRow` and `TableRow`.
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
5 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
Prevent future unsoundness that was seen in #6623.
Solution
Newtype both indexes in
ArchetypeandTableasArchetypeRowandTableRow. This avoids weird numerical manipulation on the indices, and can be stored and treated opaquely. Also enforces the source and destination of where these indices at a type level.Changelog
Changed:
Archetypeindices andTablerows have been newtyped asArchetypeRowandTableRow.