Change TableRow::new to accept u32
#10806
Labels
A-ECS
Entities, components, systems, and events
C-Code-Quality
A section of code that is hard to understand or change
Comments
alice-i-cecile
changed the title
TableRow::new to accept u32
alice-i-cecile
added
A-ECS
github-merge-queue bot
pushed a commit
that referenced
this issue
Dec 5, 2023
# Objective - Fixes #10806 ## Solution Replaced `new` and `index` methods for both `TableRow` and `TableId` with `from_*` and `as_*` methods. These remove the need to perform casting at call sites, reducing the total number of casts in the Bevy codebase. Within these methods, an appropriate `debug_assertion` ensures the cast will behave in an expected manner (no wrapping, etc.). I am using a `debug_assertion` instead of an `assert` to reduce any possible runtime overhead, however minimal. This choice is something I am open to changing (or leaving up to another PR) if anyone has any strong arguments for it. --- ## Changelog - `ComponentSparseSet::sparse` stores a `TableRow` instead of a `u32` (private change) - Replaced `TableRow::new` and `TableRow::index` methods with `TableRow::from_*` and `TableRow::as_*`, with `debug_assertions` protecting any internal casting. - Replaced `TableId::new` and `TableId::index` methods with `TableId::from_*` and `TableId::as_*`, with `debug_assertions` protecting any internal casting. - All `TableId` methods are now `const` ## Migration Guide - `TableRow::new` -> `TableRow::from_usize` - `TableRow::index` -> `TableRow::as_usize` - `TableId::new` -> `TableId::from_usize` - `TableId::index` -> `TableId::as_usize` --- ## Notes I have chosen to remove the `index` and `new` methods for the following chain of reasoning: - Across the codebase, `new` was called with a mixture of `u32` and `usize` values. Likewise for `index`. - Choosing `new` to either be `usize` or `u32` would break half of these call-sites, requiring `as` casting at the site. - Adding a second method `new_u32` or `new_usize` avoids the above, bu looks visually inconsistent. - Therefore, they should be replaced with `from_*` and `as_*` methods instead. Worth noting is that by updating `ComponentSparseSet`, there are now zero instances of interacting with the inner value of `TableRow` as a `u32`, it is exclusively used as a `usize` value (due to interactions with methods like `len` and slice indexing). I have left the `as_u32` and `from_u32` methods as the "proper" constructors/getters.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Which also suggests instead of this PR we should probably:
TableRow::newto acceptu32TableRowto various places likeComponentSparseSet.sparseOriginally posted by @stepancheg in #10794 (comment)
The text was updated successfully, but these errors were encountered: