Remove num_traits dependency
#3373
Conversation
robertbastian
force-pushed
the
num_traits
branch
from
5d03255 to
5febab2
Compare
This comment was marked as spam.
This comment was marked as spam.
robertbastian
requested review from
a team,
sffc,
aethanyc and
makotokato
as code owners
| @@ -72,7 +72,7 @@ pub const EXTRA_EXPERIMENTAL_DEPS: &[&str] = &[ | |||
| ]; | |||
|
|
|||
| /// Dependencies allowed when opting in to LSTM segmenter | |||
| pub const EXTRA_LSTM_DEPS: &[&str] = &["libm", "num-traits"]; | |||
| pub const EXTRA_LSTM_DEPS: &[&str] = &["libm"]; | |||
There was a problem hiding this comment.
praise: thanks for updating this.
(I have plans to make this file complain if an allowlisted dep goes completely unused after all of the checks, but it's not priority)
| // let clients provide these functions. | ||
| impl CoreFloat for f32 { | ||
| fn exp(self) -> Self { | ||
| libm::expf(self) |
There was a problem hiding this comment.
Question: before we introduced libm via num-traits, we had been using the f32::exp function. Now we're unconditionally using the libm version. Explain?
There was a problem hiding this comment.
Are we? In std mode this would fall back to f32.exp(), no?
There was a problem hiding this comment.
Before we were either using the inherent f32::exp, or num_traits::Float::f32, which is backed by libm. This is doing the exact same thing.
There was a problem hiding this comment.
Before we introduced num_traits we weren't no_std. With std, function resolution will use f32::exp, without it will use CoreFloat::exp.
Similar to #3369, the
num_traitsdependency doesn't add anything, and we only pull it in to getlibm.Ideally we wouldn't need that dependency, but rust-lang/rfcs#2505 isn't moving. Currently Rust is in the unfortunate position that any
#[no_std]build needs to use the Rustlibmimplementation, even if a more efficient native libm is available.