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

Revive Float+Real in no_std thanks to libm #99

Merged
merged 19 commits into from
Sep 30, 2019
Merged

Revive Float+Real in no_std thanks to libm #99

merged 19 commits into from
Sep 30, 2019

Conversation

yoanlcq
Copy link
Contributor

@yoanlcq yoanlcq commented Feb 3, 2019

Greetings,

This is a hopeful fix for #75.
Basically: Add libm as an optional dependency, and handle three possible cases depending on which features are enabled:

  • std and libm: std is used;
  • std and not libm: std is used;
  • libm and not std: libm and FloatCore are used.

It was briefly mentioned that libm wasn't ready yet, but this was months ago, and I believe it is better not to wait for too long.
If anything, bugs in libm should be fixed in libm; num-traits is only delegating its implementations to it; not to mention that the more libm is used, the likelier issues are to be found and hopefully fixed.

Thanks in advance!

@yoanlcq
Copy link
Contributor Author

yoanlcq commented Feb 3, 2019

That should do; hopefully I didn't forget anything :)
EDIT: Whoops doc-tests

@yoanlcq
Copy link
Contributor Author

yoanlcq commented Feb 3, 2019

CI fails because of overflow issues with libm. So I guess this is blocked by rust-lang/libm#4.
Apparently libm also uses some post-1.8 features which cause the 1.8 build to fail, but these look easy to fix.

src/real.rs Outdated Show resolved Hide resolved
@cuviper
Copy link
Member

cuviper commented Feb 12, 2019

Thanks for doing this - it looks great! But yes, the libm overflows are a problem...

@Schultzer
Copy link

Hi @yoanlcq and @cuviper a new version of libm has been released and it seams that the overflow problem with the tests in debug mode has been resolved any chance that we can move forward with this? :)

@korken89
Copy link

Hi, any progress on this? cc @cuviper

bors bot added a commit to Ogeon/palette that referenced this pull request Aug 11, 2019
142: Make libm optional r=Ogeon a=Ogeon

I decided to do this to avoid having it as an unused dependency and also to hopefully be compatible with a future release of `num_traits`, where they seem to be planning the same kind of construct, judging by rust-num/num-traits#99. A transition to using `num_trait` for both cases will hopefully be seamless for the users after this.

I think this also fixes #116.

Co-authored-by: Erik Hedvall <erikwhedvall@gmail.com>
@CryZe
Copy link
Contributor

CryZe commented Sep 4, 2019

I'd like to see this move forward too, as there's quite a few crates that I'd like to send PRs to for no_std support that depend on this.

@yoanlcq
Copy link
Contributor Author

yoanlcq commented Sep 4, 2019

I think it's pretty much ready now; the only problem seems to be that libm doesn't compile on some older Rust versions supported by num-traits.

(Edit: For information, I'm not really planning to investigate these errors from libm right now).

@korken89
Copy link

korken89 commented Sep 4, 2019

Awesome, thanks for doing the work!

@cuviper
Copy link
Member

cuviper commented Sep 4, 2019

the only problem seems to be that libm doesn't compile on some older Rust versions supported by num-traits.

I'm OK with that -- this is new functionality for no_std users, so it's fine for this to have less compatibility. Just document that it's limited by the MSRV of the libm crate, and adjust the CI script accordingly (like i128).

@cuviper cuviper removed the blocked label Sep 6, 2019
@yoanlcq
Copy link
Contributor Author

yoanlcq commented Sep 11, 2019

That should do, I hope :) Sorry for the delay.

@CryZe
Copy link
Contributor

CryZe commented Sep 26, 2019

What's the status of this? This would unblock a lot of PRs / releases for other crates.

@korken89
Copy link

It should be ready for merge, so I think we are waiting for the maintainers

@cuviper
Copy link
Member

cuviper commented Sep 27, 2019

I rebased and tweaked a few things, e.g. Real doesn't need all those doc-test guards now that we're guarding the trait entirely. I think we're ready, but I'd appreciate if anyone else takes another look.

@yoanlcq
Copy link
Contributor Author

yoanlcq commented Sep 29, 2019

The single guard in mod real is so obviously better, that I wonder why I didn't do it this way in the first place...

Anyway, I couldn't find anything "wrong" in these last changes. I would pretty confidently say "ship it"!

@cuviper
Copy link
Member

cuviper commented Sep 30, 2019

The single guard in mod real is so obviously better, that I wonder why I didn't do it this way in the first place...

I think because you had the trait itself unguarded at first, only the impl depending on std/libm. So all those doctests had to be guarded whether there was any useful type available to test.

bors r+

bors bot added a commit that referenced this pull request Sep 30, 2019
99: Revive Float+Real in no_std thanks to libm r=cuviper a=yoanlcq

Greetings,

This is a hopeful fix for #75.  
Basically: Add `libm` as an optional dependency, and handle three possible cases depending on which features are enabled:
- std and libm: std is used;
- std and not libm: std is used;
- libm and not std: libm and FloatCore are used.

It was briefly mentioned that `libm` wasn't ready yet, but this was months ago, and I believe it is better not to wait for too long.  
If anything, bugs in `libm` should be fixed in `libm`; `num-traits` is only delegating its implementations to it; not to mention that the more `libm` is used, the likelier issues are to be found and hopefully fixed.

Thanks in advance!

Co-authored-by: Yoan Lecoq <yoanlecoq.io@gmail.com>
Co-authored-by: Josh Stone <cuviper@gmail.com>
@bors
Copy link
Contributor

bors bot commented Sep 30, 2019

Build succeeded

@bors bors bot merged commit 93328df into rust-num:master Sep 30, 2019
bors bot added a commit to blckngm/titun that referenced this pull request Nov 13, 2019
62: Bump num-traits from 0.2.8 to 0.2.9 r=sopium a=dependabot-preview[bot]

Bumps [num-traits](https://github.com/rust-num/num-traits) from 0.2.8 to 0.2.9.
<details>
<summary>Changelog</summary>

*Sourced from [num-traits's changelog](https://github.com/rust-num/num-traits/blob/master/RELEASES.md).*

> # Release 0.2.9 (2019-11-12)
> 
> - [A new optional `libm` dependency][99] enables the `Float` and `Real` traits
>   in `no_std` builds.
> - [The new `clamp_min` and `clamp_max`][122] limit minimum and maximum values
>   while preserving input `NAN`s.
> - [Fixed a panic in floating point `from_str_radix` on invalid signs][126].
> - Miscellaneous documentation updates.
> 
> **Contributors**: [@&#8203;cuviper](https://github.com/cuviper), [@&#8203;dingelish](https://github.com/dingelish), [@&#8203;HeroicKatora](https://github.com/HeroicKatora), [@&#8203;jturner314](https://github.com/jturner314), [@&#8203;ocstl](https://github.com/ocstl),
> [@&#8203;Shnatsel](https://github.com/Shnatsel), [@&#8203;termoshtt](https://github.com/termoshtt), [@&#8203;waywardmonkeys](https://github.com/waywardmonkeys), [@&#8203;yoanlcq](https://github.com/yoanlcq)
> 
> [99]: [rust-num/num-traits#99](https://github-redirect.dependabot.com/rust-num/num-traits/pull/99)
> [122]: [rust-num/num-traits#122](https://github-redirect.dependabot.com/rust-num/num-traits/pull/122)
> [126]: [rust-num/num-traits#126](https://github-redirect.dependabot.com/rust-num/num-traits/pull/126)
</details>
<details>
<summary>Commits</summary>

- [`8eb83e2`](rust-num/num-traits@8eb83e2) Merge [#142](https://github-redirect.dependabot.com/rust-num/num-traits/issues/142)
- [`b61498a`](rust-num/num-traits@b61498a) Release 0.2.9
- [`6408853`](rust-num/num-traits@6408853) Remove unused #[inline] attributes
- [`4cbc27d`](rust-num/num-traits@4cbc27d) Merge [#137](https://github-redirect.dependabot.com/rust-num/num-traits/issues/137)
- [`da66427`](rust-num/num-traits@da66427) Merge [#140](https://github-redirect.dependabot.com/rust-num/num-traits/issues/140)
- [`da19eaf`](rust-num/num-traits@da19eaf) Compatible with cargo --remap-path-prefix
- [`fdb4181`](rust-num/num-traits@fdb4181) ToPrimitive: document when `to_` methods can return `None`
- [`27e3f85`](rust-num/num-traits@27e3f85) NumCast: document when `from` can return `None`
- [`16cdce3`](rust-num/num-traits@16cdce3) FromPrimitive: fix some comments
- [`2f0cffd`](rust-num/num-traits@2f0cffd) Merge [#99](https://github-redirect.dependabot.com/rust-num/num-traits/issues/99)
- Additional commits viewable in [compare view](rust-num/num-traits@num-traits-0.2.8...num-traits-0.2.9)
</details>
<br />

[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=num-traits&package-manager=cargo&previous-version=0.2.8&new-version=0.2.9)](https://dependabot.com/compatibility-score.html?dependency-name=num-traits&package-manager=cargo&previous-version=0.2.8&new-version=0.2.9)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language
- `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language
- `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language
- `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com):
- Update frequency (including time of day and day of week)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)



</details>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
bors bot added a commit to blckngm/titun that referenced this pull request Nov 13, 2019
62: Bump num-traits from 0.2.8 to 0.2.9 r=sopium a=dependabot-preview[bot]

Bumps [num-traits](https://github.com/rust-num/num-traits) from 0.2.8 to 0.2.9.
<details>
<summary>Changelog</summary>

*Sourced from [num-traits's changelog](https://github.com/rust-num/num-traits/blob/master/RELEASES.md).*

> # Release 0.2.9 (2019-11-12)
> 
> - [A new optional `libm` dependency][99] enables the `Float` and `Real` traits
>   in `no_std` builds.
> - [The new `clamp_min` and `clamp_max`][122] limit minimum and maximum values
>   while preserving input `NAN`s.
> - [Fixed a panic in floating point `from_str_radix` on invalid signs][126].
> - Miscellaneous documentation updates.
> 
> **Contributors**: [@&#8203;cuviper](https://github.com/cuviper), [@&#8203;dingelish](https://github.com/dingelish), [@&#8203;HeroicKatora](https://github.com/HeroicKatora), [@&#8203;jturner314](https://github.com/jturner314), [@&#8203;ocstl](https://github.com/ocstl),
> [@&#8203;Shnatsel](https://github.com/Shnatsel), [@&#8203;termoshtt](https://github.com/termoshtt), [@&#8203;waywardmonkeys](https://github.com/waywardmonkeys), [@&#8203;yoanlcq](https://github.com/yoanlcq)
> 
> [99]: [rust-num/num-traits#99](https://github-redirect.dependabot.com/rust-num/num-traits/pull/99)
> [122]: [rust-num/num-traits#122](https://github-redirect.dependabot.com/rust-num/num-traits/pull/122)
> [126]: [rust-num/num-traits#126](https://github-redirect.dependabot.com/rust-num/num-traits/pull/126)
</details>
<details>
<summary>Commits</summary>

- [`8eb83e2`](rust-num/num-traits@8eb83e2) Merge [#142](https://github-redirect.dependabot.com/rust-num/num-traits/issues/142)
- [`b61498a`](rust-num/num-traits@b61498a) Release 0.2.9
- [`6408853`](rust-num/num-traits@6408853) Remove unused #[inline] attributes
- [`4cbc27d`](rust-num/num-traits@4cbc27d) Merge [#137](https://github-redirect.dependabot.com/rust-num/num-traits/issues/137)
- [`da66427`](rust-num/num-traits@da66427) Merge [#140](https://github-redirect.dependabot.com/rust-num/num-traits/issues/140)
- [`da19eaf`](rust-num/num-traits@da19eaf) Compatible with cargo --remap-path-prefix
- [`fdb4181`](rust-num/num-traits@fdb4181) ToPrimitive: document when `to_` methods can return `None`
- [`27e3f85`](rust-num/num-traits@27e3f85) NumCast: document when `from` can return `None`
- [`16cdce3`](rust-num/num-traits@16cdce3) FromPrimitive: fix some comments
- [`2f0cffd`](rust-num/num-traits@2f0cffd) Merge [#99](https://github-redirect.dependabot.com/rust-num/num-traits/issues/99)
- Additional commits viewable in [compare view](rust-num/num-traits@num-traits-0.2.8...num-traits-0.2.9)
</details>
<br />

[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=num-traits&package-manager=cargo&previous-version=0.2.8&new-version=0.2.9)](https://dependabot.com/compatibility-score.html?dependency-name=num-traits&package-manager=cargo&previous-version=0.2.8&new-version=0.2.9)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language
- `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language
- `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language
- `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com):
- Update frequency (including time of day and day of week)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)



</details>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
ghost pushed a commit to tommilligan/multiplicative-persistence that referenced this pull request Nov 13, 2019
25: Bump num-traits from 0.2.8 to 0.2.9 r=tommilligan a=dependabot-preview[bot]

Bumps [num-traits](https://github.com/rust-num/num-traits) from 0.2.8 to 0.2.9.
<details>
<summary>Changelog</summary>

*Sourced from [num-traits's changelog](https://github.com/rust-num/num-traits/blob/master/RELEASES.md).*

> # Release 0.2.9 (2019-11-12)
> 
> - [A new optional `libm` dependency][99] enables the `Float` and `Real` traits
>   in `no_std` builds.
> - [The new `clamp_min` and `clamp_max`][122] limit minimum and maximum values
>   while preserving input `NAN`s.
> - [Fixed a panic in floating point `from_str_radix` on invalid signs][126].
> - Miscellaneous documentation updates.
> 
> **Contributors**: [@&#8203;cuviper](https://github.com/cuviper), [@&#8203;dingelish](https://github.com/dingelish), [@&#8203;HeroicKatora](https://github.com/HeroicKatora), [@&#8203;jturner314](https://github.com/jturner314), [@&#8203;ocstl](https://github.com/ocstl),
> [@&#8203;Shnatsel](https://github.com/Shnatsel), [@&#8203;termoshtt](https://github.com/termoshtt), [@&#8203;waywardmonkeys](https://github.com/waywardmonkeys), [@&#8203;yoanlcq](https://github.com/yoanlcq)
> 
> [99]: [rust-num/num-traits#99](https://github-redirect.dependabot.com/rust-num/num-traits/pull/99)
> [122]: [rust-num/num-traits#122](https://github-redirect.dependabot.com/rust-num/num-traits/pull/122)
> [126]: [rust-num/num-traits#126](https://github-redirect.dependabot.com/rust-num/num-traits/pull/126)
</details>
<details>
<summary>Commits</summary>

- [`8eb83e2`](rust-num/num-traits@8eb83e2) Merge [#142](https://github-redirect.dependabot.com/rust-num/num-traits/issues/142)
- [`b61498a`](rust-num/num-traits@b61498a) Release 0.2.9
- [`6408853`](rust-num/num-traits@6408853) Remove unused #[inline] attributes
- [`4cbc27d`](rust-num/num-traits@4cbc27d) Merge [#137](https://github-redirect.dependabot.com/rust-num/num-traits/issues/137)
- [`da66427`](rust-num/num-traits@da66427) Merge [#140](https://github-redirect.dependabot.com/rust-num/num-traits/issues/140)
- [`da19eaf`](rust-num/num-traits@da19eaf) Compatible with cargo --remap-path-prefix
- [`fdb4181`](rust-num/num-traits@fdb4181) ToPrimitive: document when `to_` methods can return `None`
- [`27e3f85`](rust-num/num-traits@27e3f85) NumCast: document when `from` can return `None`
- [`16cdce3`](rust-num/num-traits@16cdce3) FromPrimitive: fix some comments
- [`2f0cffd`](rust-num/num-traits@2f0cffd) Merge [#99](https://github-redirect.dependabot.com/rust-num/num-traits/issues/99)
- Additional commits viewable in [compare view](rust-num/num-traits@num-traits-0.2.8...num-traits-0.2.9)
</details>
<br />

[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=num-traits&package-manager=cargo&previous-version=0.2.8&new-version=0.2.9)](https://dependabot.com/compatibility-score.html?dependency-name=num-traits&package-manager=cargo&previous-version=0.2.8&new-version=0.2.9)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language
- `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language
- `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language
- `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com):
- Update frequency (including time of day and day of week)
- Pull request limits (per update run and/or open at any time)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)



</details>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
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.

5 participants