Skip to content

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tgross35
Copy link

@tgross35 tgross35 commented May 30, 2025

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 when these values are the same as Exponent::{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

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
@tgross35
Copy link
Author

Would something like from_raw_parts_u64 or a AsWords trait be useful/welcome? In order to allow using the same words on both 32- and 64-bit platforms.

@stencillogic
Copy link
Owner

Thank you for raising the PR.

@stencillogic
Copy link
Owner

Would something like from_raw_parts_u64 or a AsWords trait be useful/welcome? In order to allow using the same words on both 32- and 64-bit platforms.

I don't quite understand the idea. What problem would it solve? Platform independent serialization / deserialization?

@tgross35
Copy link
Author

tgross35 commented Jun 6, 2025

I was thinking of making it simpler for cases like this:

#[cfg(target_pointer_width = "64")]
let words = [
14741299514016743161,
12241124915312604155,
15616730352774362773,
12672098147611000049,
12535862302449814170,
];
#[cfg(not(target_pointer_width = "64"))]
let words = [
614153977, 3432226254, 342111227, 2850108993, 3459069589, 3636053379, 658324721,
2950452768, 2730183322, 2918732888,
];

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?

@stencillogic
Copy link
Owner

I was thinking of making it simpler for cases like this:
...
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 #[cfg(test)] and used solely in tests.

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.

Use target pointer width rather than x86
2 participants