-
Notifications
You must be signed in to change notification settings - Fork 7
Reduce the x86-specificity of the crate #36
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
base: main
Are you sure you want to change the base?
Conversation
Currently a lot of things are gated on `cfg(target_arch = "x86")`, which is used to indicate systems with 32-bit words. Apply the following changes to make this less specific: * Replace x86-32 configuration with configuration based on `target_pointer_width`. * Where possible, replace `#[cfg]` with `cfg!` or eliminate the check, to increase the percentage of code that gets validated on any platform. * Remove some configuration that was unneeded, for the same reason. This includes things like `#[cfg(target_arch = "x86")]` on comparisons to `EXPONENT_MIN` or `EXPONENT_MAX`. The checks are only useful on 32-bit platforms, but this is a trivial compiler optimization so not much is gained by keeping the config. I have verified that the crate builds on armv7 targets (but have not tested). Fixes: stencillogic#26
|
Would something like |
|
Thank you for raising the PR. |
I don't quite understand the idea. What problem would it solve? Platform independent serialization / deserialization? |
|
I was thinking of making it simpler for cases like this: astro-float/astro-float-num/src/ops/consts/e.rs Lines 143 to 156 in dd15e90
The arrays contain the same data, the 32-bit version just has to be split in half. A helper function could accept a u64 array on either platform and just reinterpret as needed. I guess this might not be too useful outside of writing platform-agnostic tests? |
Yes, it makes sense. There are several functions like this in the crate marked with |
| *r = s as Word; | ||
| 0 | ||
| } | ||
| #[allow(unreachable_code)] // not used on x86 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for marking the code unreachable? Wouldn't it be better to keep it as it was before, since the code could be reachable for architectures different from x86, x86_64?
| ret.m = m; | ||
| ret.e = exponent - 0b1111111111 - shift as Exponent; | ||
|
|
||
| #[cfg(target_arch = "x86")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't it cause a warning from compiler about nonsense check if target_pointer_width = "64" ? If target_pointer_width = "64" then the condition is always true since EXPONENT_MAX == Exponent::MAX, and EXPONENT_MIN == Exponent::MIN. But for target_pointer_width = "32" it can return false.
This statement is fair for all the places in the code where the line "#[cfg(target_arch = "x86")]" has been removed.
Currently a lot of things are gated on
cfg(target_arch = "x86"), which is used to indicate systems with 32-bit words. Apply the following changes to make this less specific:target_pointer_width.#[cfg]withcfg!or eliminate the check, to increase the percentage of code that gets validated on any platform.#[cfg(target_arch = "x86")]on comparisons toEXPONENT_MINorEXPONENT_MAX. The checks are only useful on 32-bit platforms, but this is a trivial compiler optimization when these values are the same asExponent::{MIN,MAX}so not much is gained by keeping the config.I have verified that the crate builds on armv7 targets (but have not tested).
Fixes: #26