traits: Introduce std feature #296

vks · 2017-05-31T11:45:58Z

This makes it possible to build traits without std. For this a new
trait BasicFloat was introduced, implementing some basic functionality
that works with core. Most notably this is lacking functions like
cos, sin, etc.

Float is not available without std.

Refs #216.

This makes it possible to build `traits` without `std`. For this a new trait `BasicFloat` was introduced, implementing some basic functionality that works with `core`. Most notably this is lacking functions like `cos`, `sin`, etc. `Float` is not available without `std`. Refs rust-num#216.

cuviper · 2017-05-31T19:32:20Z

There's a CI failure reported across all versions. I think this probably needs some direct no-std testing in ci/test_full.sh too.

Did you consider just gating #[cfg(feature = "std")] on Float methods as needed? I know that makes a lot more places that have to be marked, but I think it's easier to explain. I don't like the method duplication, and I don't want users to be confused about when to use Float or BasicFloat.

If you really want to have two separate traits, maybe it would be better to have Float: BasicFloat so they are directly related.

cuviper · 2017-05-31T19:33:45Z

maybe it would be better to have Float: BasicFloat

Hmm, but that's a breaking change for implementors. :/

vks · 2017-05-31T21:02:03Z

Hmm, but that's a breaking change for implementors. :/

Yes, I considered that, but discarded it for this reason.

I like your suggestion of gating the methods, this would have the advantage that there will be no redundant reimplementation of the methods currently in BasicFloat if std is available.

Set up AppVeyor

This removes the `BasicFloat` trait.

vks · 2017-06-02T09:58:00Z

I did what you suggested, which coincidentally also fixed the tests.

I added quite a few default implementations to Float. They only matter for no_std at the moment.

cuviper

Looks good, just a few tweaks requested.

We should probably also add a caution in the docs for implementors of Float. Those that implement it normally are fine, but if someone now chooses to implement Float only for no_std, their build will fail if anyone else in the entire dependency graph enables num-traits/std.

cuviper · 2017-06-02T17:43:25Z

ci/test_full.sh

@@ -10,6 +10,10 @@ for package in bigint complex integer iter rational traits; do
  cargo test --manifest-path $package/Cargo.toml
 done

+# Only num-tratis supports no_std at the moment.


sp "tratis"

cuviper · 2017-06-02T18:00:17Z

traits/src/float.rs

+            }
+        }
+        acc
+    }


Maybe use the generic num_traits::pow() once exp is positive? I see your implementation is identical, but we don't need to duplicate code.

The problem is that num_traits::pow takes a usize for the exponent, which might be smaller than i32 in the future. That is why I duplicated the implementation.

If we made a breaking change, then we could consolidate the two implementations by making num_traits::pow use a u32 for the exponent.

Hmm, do 16-bit targets even have these floats? But OK, not a big deal.

Probably not. I could use num_traits::pow and cast, hoping it works. Alternatively we could use a macro to deduplicate the implementation.

Or instead of casting, use FromPrimitive and unwrap.

cuviper · 2017-06-02T18:02:55Z

traits/src/float.rs

+    /// assert!(abs_difference < 1e-10);
+    /// ```
+    #[cfg(not(feature = "std"))]
+    fn to_degrees(self) -> Self;


Ugh, that's annoying to have a separate declaration, but I don't see a better way, other than excluding it from no_std entirely.

cuviper · 2017-06-02T18:05:19Z

traits/src/float.rs

@@ -1097,117 +1287,138 @@ macro_rules! float_impl {
            fn to_degrees(self) -> Self {
                // NB: `f32` didn't stabilize this until 1.7
                // <$T>::to_degrees(self)
-                self * (180. / ::std::$T::consts::PI)
+                self * (180. / ::core::$T::consts::PI)


Our baseline is now 1.8 - can we just use the native implementation?

cuviper · 2017-06-02T18:05:48Z

traits/src/float.rs

            }

            #[inline]
            fn to_radians(self) -> Self {
                // NB: `f32` didn't stabilize this until 1.7
                // <$T>::to_radians(self)
-                self * (::std::$T::consts::PI / 180.)
+                self * (::core::$T::consts::PI / 180.)


Ditto on using the native impl.

vks · 2017-06-07T10:25:08Z

@cuviper I think I addressed all your comments.

cuviper · 2017-06-07T23:12:16Z

traits/src/float.rs

-                // NB: `f32` didn't stabilize this until 1.7
-                // <$T>::to_degrees(self)
-                self * (180. / ::std::$T::consts::PI)
+                <$T>::to_degrees(self)


Oh, this doesn't work for no_std, because the inherent methods are in std. So it recurses:

warning: function cannot return without recurring --> src\float.rs:1272:13 | 1272 | fn to_degrees(self) -> Self { | _____________^ starting here... 1273 | | <$T>::to_degrees(self) 1274 | | } | |_____________^ ...ending here ... 1482 | float_impl!(f32 integer_decode_f32 classify_f32); | ------------------------------------------------- in this macro invocation | = note: #[warn(unconditional_recursion)] on by default

I guess we have to roll our own, but it's not a big deal since these are trivial.

Can you add #![deny(unconditional_recursion)] to the crate? This seems like a problem we're vulnerable to in general, with so many traits that are supposed to forward calls...

Sorry, I actually wanted to keep the implementation for no_std but forgot about it...

Should be fixed in the most recent commit.

cuviper

Looks great, thanks!

cuviper · 2017-06-09T06:25:01Z

@homu r+

homu · 2017-06-09T06:25:03Z

📌 Commit ba73ba2 has been approved by cuviper

traits: Introduce std feature This makes it possible to build `traits` without `std`. For this a new trait `BasicFloat` was introduced, implementing some basic functionality that works with `core`. Most notably this is lacking functions like `cos`, `sin`, etc. `Float` is not available without `std`. Refs #216.

homu · 2017-06-09T06:25:14Z

⌛ Testing commit ba73ba2 with merge 8b5d4ac...

homu · 2017-06-09T06:33:27Z

☀️ Test successful - status

nox · 2017-06-09T10:43:20Z

This is a breaking change and the release should be yanked. Feature-gating things that were previously not feature-gated is a breaking change.

This reverts commit 8b5d4ac, reversing changes made to ef752e4.

Revert "Auto merge of #296 - vks:no_std, r=cuviper" This reverts commit 8b5d4ac, reversing changes made to ef752e4. See #297 -- it's a breaking change to feature-gate existing APIs.

@vks

This is a port of @vks's rust-num/num#296, but without the feature- toggled changes to `Float`.

@vks

30: Re-introduce the std feature r=vks a=cuviper This is a port of @vks's rust-num/num#296, but without the feature-toggled changes to `Float`. Now `Float` and the newer `Real` are completely dependent on having `std` enabled. In the future we can consider adding separate more-limited float/real traits that can work without `std`, like the `BaseFloat` that was originally proposed in the former PR. This is a breaking change with a bump to 0.2, since anyone currently using `default-features = false` will lose functionality. The actual API is otherwise unchanged, so my plan is to employ the "semver trick" -- publishing a new num-traits-0.1 that re-exports everything from 0.2 (with `std`). Thus all `num-traits` users should remain compatible even if they mix 0.1 and 0.2. Closes #16.

remexre pushed a commit to remexre/num that referenced this pull request Jun 1, 2017

Merge pull request rust-num#296 from serde-rs/appveyor

e8a06ef

Set up AppVeyor

vks added 2 commits June 2, 2017 11:47

Mark methods in Float whether they require std

6253669

This removes the `BasicFloat` trait.

Implement Float::{min, max} for no_std

169105b

Travis, please test traits with no_std

128e456

cuviper requested changes Jun 2, 2017

View reviewed changes

vitiral mentioned this pull request Jun 5, 2017

no_std support? bincode-org/bincode#189

Closed

vks added 4 commits June 7, 2017 11:59

Fix typo

d201882

Use native to_degrees/to_radians

f8a6196

Don't duplicate num_traits::pow

63ce0f0

Add word of caution about Float and no_std

b7d2a99

cuviper reviewed Jun 7, 2017

View reviewed changes

Avoid infinite recursion on no_std

ba73ba2

cuviper approved these changes Jun 9, 2017

View reviewed changes

homu merged commit ba73ba2 into rust-num:master Jun 9, 2017

cuviper added a commit to cuviper/num that referenced this pull request Jun 9, 2017

Revert "Auto merge of rust-num#296 - vks:no_std, r=cuviper"

fe34d17

This reverts commit 8b5d4ac, reversing changes made to ef752e4.

This was referenced Dec 19, 2017

Make std a feature, using #![no_std] otherwise rust-num/num-traits#16

Closed

nostd feature #360

Closed

termoshtt mentioned this pull request Jan 12, 2018

"std" feature rust-num/num-traits#24

Closed

cuviper pushed a commit to cuviper/num-traits that referenced this pull request Feb 1, 2018

Re-introduce the std feature

a843027

This is a port of @vks's rust-num/num#296, but without the feature- toggled changes to `Float`.

cuviper mentioned this pull request Feb 1, 2018

Re-introduce the std feature rust-num/num-traits#30

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

traits: Introduce std feature #296

traits: Introduce std feature #296

vks commented May 31, 2017

cuviper commented May 31, 2017

cuviper commented May 31, 2017

vks commented May 31, 2017

vks commented Jun 2, 2017

cuviper left a comment

cuviper Jun 2, 2017

cuviper Jun 2, 2017

vks Jun 2, 2017

cuviper Jun 2, 2017

vks Jun 4, 2017

cuviper Jun 4, 2017

cuviper Jun 2, 2017

cuviper Jun 2, 2017

cuviper Jun 2, 2017

vks commented Jun 7, 2017

cuviper Jun 7, 2017

vks Jun 8, 2017

cuviper left a comment

cuviper commented Jun 9, 2017

homu commented Jun 9, 2017

homu commented Jun 9, 2017

homu commented Jun 9, 2017

nox commented Jun 9, 2017

traits: Introduce std feature #296

traits: Introduce std feature #296

Conversation

vks commented May 31, 2017

cuviper commented May 31, 2017

cuviper commented May 31, 2017

vks commented May 31, 2017

vks commented Jun 2, 2017

cuviper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vks commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuviper left a comment

Choose a reason for hiding this comment

cuviper commented Jun 9, 2017

homu commented Jun 9, 2017

homu commented Jun 9, 2017

homu commented Jun 9, 2017

nox commented Jun 9, 2017