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

Simplify equality assertions #10988

Merged
merged 1 commit into from Dec 17, 2023
Merged

Simplify equality assertions #10988

merged 1 commit into from Dec 17, 2023

Conversation

tygyh
Copy link
Contributor

@tygyh tygyh commented Dec 15, 2023

Objective

  • Shorten assertions.

Solution

  • Replace '==' assertions with 'assert_eq()' and '!=' assertions with 'assert_ne()' .

@tygyh
Copy link
Contributor Author

tygyh commented Dec 15, 2023

If I am not misunderstanding the failing test, it is testing if Rust's convertions work as intended, not if the table works as intended. I do not see a reason for that test to be in the engine in that case.

@matiqo15 matiqo15 added the C-Code-Quality A section of code that is hard to understand or change label Dec 15, 2023
matiqo15
matiqo15 suggested changes Dec 15, 2023
View reviewed changes
crates/bevy_ecs/src/storage/table.rs Outdated
@@ -56,7 +56,7 @@ impl TableId {
/// Will panic if the provided value does not fit within a [`u32`].
#[inline]
pub const fn from_usize(index: usize) -> Self {
assert!(index as u32 as usize == index);
assert_eq!(index as u32 as usize, index);
Copy link
Member

Choose a reason for hiding this comment

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

It seems like we cannot use assert_eq! in constant functions

crates/bevy_ecs/src/storage/table.rs Outdated
@@ -116,7 +116,7 @@ impl TableRow {
/// Will panic if the provided value does not fit within a [`u32`].
#[inline]
pub const fn from_usize(index: usize) -> Self {
assert!(index as u32 as usize == index);
assert_eq!(index as u32 as usize, index);
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@matiqo15
Copy link
Member

assert_eq! can't be used in constant functions, that's why CI is failing.

@tygyh
Copy link
Contributor Author

tygyh commented Dec 15, 2023

Yes, and that is why I am saying the tests are wrong. They are testing if Rust works as intended.

@matiqo15
Copy link
Member

We check if index fits into u32 and panic if it doesn't, otherwise it would be truncated to the biggest number we can fit in u32, and that could lead to unexpected behavior. So that assert is needed.

@tygyh
Copy link
Contributor Author

tygyh commented Dec 15, 2023 •
edited

I think I understand it more now. Thank you for explaining.

@matiqo15 matiqo15 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 Dec 15, 2023
@mockersf mockersf added this pull request to the merge queue Dec 16, 2023
Merged via the queue into bevyengine:main with commit 63d17e8 Dec 17, 2023
23 checks passed
@tygyh tygyh deleted the Simplify-assertions branch December 17, 2023 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djeedai djeedai djeedai approved these changes

@mockersf mockersf mockersf approved these changes

@Nilirad Nilirad Nilirad approved these changes

@matiqo15 matiqo15 matiqo15 approved these changes

Assignees
No one assigned
Labels
C-Code-Quality A section of code that is hard to understand or change 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tygyh @matiqo15 @djeedai @mockersf @Nilirad