Skip to content

Add round_ties_even to Float/Core and Real #350

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: master
Choose a base branch
from

Conversation

bushrat011899
Copy link

@bushrat011899 bushrat011899 commented Apr 18, 2025

Objective

Since Rust 1.77, round_ties_even was added to f32 and f64 in the standard library. This function is used within WGPU, and as a part of a push for no_std support, this function will need to be shimmed. It could be shimmed within Naga, but this functionality may have value to other users of num-traits.

Solution

  • Added round_ties_even to FloatCore, Float, and Real with a default implementation (not a breaking change)
  • Added a has_round_ties_even configuration check to allow detecting round_ties_even provided by the standard library in Rust 1.77 and above (not a breaking change)
  • Added a test ensuring the fallback implementation of round_ties_even matches the implementation in std for the f32 type (assuming it is equally valid for f64)

Notes

  • The msrv_1_77 feature increases MSRV to 1.77, which is a typical arrangement (e.g., serde features will typically elevate MSRV). If/when num-traits advances past MSRV 1.77, this feature can become a no-op without any breaking changes. Resolved using build.rs and autocfg.
  • This is my first contribution to num-traits, please let me know if there's anything I can do to help!

@cuviper
Copy link
Member

cuviper commented Jun 17, 2025

  • The msrv_1_77 feature increases MSRV to 1.77, which is a typical arrangement (e.g., serde features will typically elevate MSRV). If/when num-traits advances past MSRV 1.77, this feature can become a no-op without any breaking changes.

We have our own precedent for this kind of thing -- please use autocfg in the build script instead of a cargo feature, similar to the total_cmp test that's already there.

We should also add this method to Float and Real, as these traits are related but not actually connected together.

@bushrat011899 bushrat011899 changed the title Add FloatCore::round_ties_even Add round_ties_even to Float/Core and Real Jun 18, 2025
@bushrat011899
Copy link
Author

We have our own precedent for this kind of thing -- please use autocfg in the build script instead of a cargo feature, similar to the total_cmp test that's already there.

Didn't notice that! I've added a similar test to check for round_ties_even and removed the new feature.

We should also add this method to Float and Real, as these traits are related but not actually connected together.

Makes sense! I've added it to all 3. Unfortunately they each need the default implementation provided to ensure this PR isn't a breaking change, which does triplicate the code.

@cuviper
Copy link
Member

cuviper commented Jun 18, 2025

Unfortunately they each need the default implementation provided to ensure this PR isn't a breaking change, which does triplicate the code.

Hmm, and that's not easily solved with a helper function, since the necessary implementation methods are also coming from each separate trait. Maybe a macro would be ok?

For doc purposes, can you move these directly after each fn round? The doc sidebar is already sorted by name, but the main page displays methods in declaration order.

Finally, I'd prefer this to see this PR rebased rather than having a merge commit in the middle.

@bushrat011899
Copy link
Author

Maybe a macro would be ok?

Tried and seems to work well! I've placed it in macros.rs with a little documentation explaining why it exists.

For doc purposes, can you move these directly after each fn round? The doc sidebar is already sorted by name, but the main page displays methods in declaration order.

Done!

Finally, I'd prefer this to see this PR rebased rather than having a merge commit in the middle.

Apologies, I've switched to a rebase (and squashed as most of these commits were minor amendments anyway).

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.

2 participants