num-traits icon indicating copy to clipboard operation
num-traits copied to clipboard

Add `round_ties_even` to `Float/Core` and `Real`

Open bushrat011899 opened this issue 7 months ago • 4 comments

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!

bushrat011899 avatar Apr 18 '25 11:04 bushrat011899

  • 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.

cuviper avatar Jun 17 '25 21:06 cuviper

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.

bushrat011899 avatar Jun 18 '25 00:06 bushrat011899

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.

cuviper avatar Jun 18 '25 21:06 cuviper

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).

bushrat011899 avatar Jun 19 '25 02:06 bushrat011899