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.

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

chore: Remove static_assertions dependency #10348

Merged
merged 1 commit into from
Mar 25, 2023

Conversation

relrelb
Copy link
Contributor

@relrelb relrelb commented Mar 24, 2023

static_assertions seems unmaintained, and anyway assert!() is usable in const contexts since Rust 1.57.0:
https://blog.rust-lang.org/2021/12/02/Rust-1.57.0.html#panic-in-const-contexts So simply use the suggested method instead.

Also the rustversion dependency is no longer needed because rust-lang/rust#94075 already landed in stable.

@adrian17
Copy link
Collaborator

Can we keep the concrete values for the 2 platforms?

@relrelb
Copy link
Contributor Author

relrelb commented Mar 24, 2023

Can we keep the concrete values for the 2 platforms?

Why? I thought it would be nicer to compute the sizes using current pointer size.

@adrian17
Copy link
Collaborator

adrian17 commented Mar 24, 2023

The formula happens to check out now, but it doesn't represent a consistent underlying rule. As in:

  • on 32-bit, the size upper bound comes from f64 alignment requirement (removing the Number variant would reduce the size to 12b)
  • on 64-bit, the size upper bound comes from 2xpointer size plus pointer/f64 alignment (removing the String field would reduce the size to 16b).

So possible refactoring of the structs could throw the formula out of whack (plus it's reliant on what alignment our current platforms happen to have right now).

And aside from being a "regression test", having specific values at hand makes it slightly easier to eyeball memory traffic, at least for me.

@Herschel
Copy link
Member

I agree, I prefer the explicit values in this case.

@relrelb relrelb force-pushed the static_assertions branch 2 times, most recently from 86e0b56 to eba0366 Compare March 25, 2023 06:33
@relrelb
Copy link
Contributor Author

relrelb commented Mar 25, 2023

Done.

core/src/avm2/error.rs Show resolved Hide resolved
`static_assertions` seems unmaintained, and anyway `assert!()` is
usable in `const` contexts since Rust 1.57.0:
https://blog.rust-lang.org/2021/12/02/Rust-1.57.0.html#panic-in-const-contexts
So simply use the suggested method instead.

Also the `rustversion` dependency is no longer needed because
rust-lang/rust#94075 already landed in stable.
@relrelb relrelb force-pushed the static_assertions branch from eba0366 to 384af89 Compare March 25, 2023 11:57
@relrelb relrelb enabled auto-merge (rebase) March 25, 2023 11:58
@relrelb relrelb merged commit 7cdac78 into ruffle-rs:master Mar 25, 2023
@relrelb relrelb deleted the static_assertions branch March 25, 2023 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants