Skip to content
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

Float should require FloatCore #197

Open
vadixidav opened this issue Dec 12, 2020 · 5 comments
Open

Float should require FloatCore #197

vadixidav opened this issue Dec 12, 2020 · 5 comments

Comments

@vadixidav
Copy link

I am currently working on converting ndarray to no_std, but this has resulted in one particular pain point: rust-ndarray/ndarray#708 (comment).

The underlying issue is that ndarray uses Float currently, but Float does not require the implementation of FloatCore. This means that it is possible for a downstream crate to implement Float on a type without implementing FloatCore. Therefore, ndarray cannot "lower" the requirements to only FloatCore, since we cant guarantee that something that is Float is also FloatCore.

Due to this, ndarray may make a breaking change (thats up to @bluss though), but it might make sense to make this breaking change in num-traits instead. If this is a change that should be made at all, it should probably be done sooner rather than later to avoid more ecosystem pain. It could also be that we don't ever change it so that we don't need to worry about breaking any downstream code. From here on out crates switching to no_std will continue to be broken if they depend on Float, but if the change is made here in num-traits then it will break everyone at least once, which is also a huge pain, so perhaps we shouldn't do it at all.

Aside from this breaking issue, it would also make the most sense for something implementing Float to also implement FloatCore, otherwise upstream crates (like ndarray) supporting no_std would be unable to use just Float in their APIs that might call shared code (DRY principle) that works on std and no_std that uses FloatCore since Float doesn't implement FloatCore. So effectively these kinds of crates would have to have APIs with the bound Float + FloatCore, which is a bit awkward (but it would work!), or they might have awkward duplicate (or macro-generated) code.

Any thoughts?

@bluss
Copy link
Contributor

bluss commented Dec 12, 2020

(This doesn't need to detract from this issue, but:) Making ndarry no_std is a breaking change in ndarray anyway, because we're going to make the "no default features" version of ndarray be no-std. (This is a quite usual way to do it and it looks like the most reasonable solution to me.)

@SparrowLii
Copy link
Contributor

SparrowLii commented Dec 13, 2020

Also a little curious. I think the reason why Float does not implement FloatCore is to express that FloatCore is the no_std version of Float, so there will be differences in implementation or functionality (even if not now)? If we want to express that FloatCore is the core function of Float, then Float should implement FloatCore, personal opinion.

@bluss
Copy link
Contributor

bluss commented Dec 13, 2020

It is hard to make breaking changes in this crate. Possible breaking changes are tracked here: #47

@cuviper
Copy link
Member

cuviper commented Dec 14, 2020

Due to this, ndarray may make a breaking change (thats up to @bluss though), but it might make sense to make this breaking change in num-traits instead.

There's no "instead", really -- if we make a breaking change in num-traits with a requisite semver bump (1.0 or otherwise), ndarray will also have to break to follow along.

@vadixidav
Copy link
Author

@cuviper True, but if a breaking change is made here at some point, it would be better to include it to avoid this situation across multiple downstream crates trying to use no_std. Otherwise it probably isn't worth it.

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

No branches or pull requests

4 participants