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

no_std implemenatation #15

Closed
wants to merge 15 commits into from
Closed

no_std implemenatation #15

wants to merge 15 commits into from

Conversation

termoshtt
Copy link
Contributor

@termoshtt termoshtt commented Feb 22, 2018

It's an attempt to use num-complex in no_std setting.
I have split src/lib.rs into src/{lib,math,fmt,from_str}.rs for incremental testing. These submodules are disabled when no_std because

  • math needs std mathematical functions (e.g. atan2)
  • fmt needs format!
  • from_str needs String
    • Num is included here due to from_str_radix

Basically, I did not modify any implementations of each functions.
This PR depends FloatCore rust-num/num-traits#32 which is not included in num-traits 0.2, and use git and rev in Cargo.toml. Float/FloatCore are switched in lib.rs (FloatCore as Float) for common functions e.g. is_infinite(), and math module uses Float only.

Sorry for inconvenient diff due to file splitting.
Thanks.

@cuviper
Copy link
Member

cuviper commented Feb 22, 2018

First reactions:

I have split src/lib.rs into ...

It does make this harder to review, but in general I really like having smaller more-focused source files. I ought to do that throughout more of num...

  • math needs std mathematical functions (e.g. atan2)

Ok, that's expected.

  • fmt needs format!

That's unfortunate. Perhaps we can rewrite this with format_args! temporaries instead?

  • from_str needs String
    • Num is included here due to from_str_radix

Ouch, losing Num hurts. I think maybe we can work around that too though. IIRC the main reason for the temporary string was for that comment to "ignore whitespace between minus sign and next number." Perhaps instead we can handle that case by parsing the number as positive, and then negating it. (using zero() - b since we don't have Neg.)

This PR depends FloatCore rust-num/num-traits#32 which is not included in num-traits 0.2, and use git and rev in Cargo.toml. Float/FloatCore are switched in lib.rs (FloatCore as Float) for common functions e.g. is_infinite(),

Using FloatCore is fine -- I need to finish that up and publish it. I think switching it is problematic though, because features are additive. std could be enabled by other parts of the dependency graph, thus breaking a crate that only requested num-complex default-features = false, expecting it to use FloatCore.

@termoshtt
Copy link
Contributor Author

Thanks comment.

That's unfortunate. Perhaps we can rewrite this with format_args! temporaries instead?

I missed core::format_args!. I will try it.

Ouch, losing Num hurts.

I think so. I will try to rewrite from_str as compatible with no_std. Thanks your information about the implementation.

I think switching it is problematic though, because features are additive.

I see your concern. They should be separated as you say.

It does make this harder to review, but in general I really like having smaller more-focused source files.

I think I should pull out the file split part into another PR. The submodules described above help us considering the no_std compatibility respectively, and re-merging them is not a good way I think.

@cuviper
Copy link
Member

cuviper commented Mar 6, 2018

If you're ready to split up your PR as you mentioned, num-traits 0.2.1 now contains FloatCore.

cuviper added a commit to cuviper/num-complex that referenced this pull request May 10, 2018
Based on @termoshtt's rust-num#15, without the intrusive file splitting.  I also
addressed the other review comments I had left there.  The limitations
of `no_std` are the missing implementations of `Error` and methods based
on `Float`, although `FloatCore` is now used for a few things.  Format
widths are also not supported, as I couldn't find a way to do this
without using a temporary `String`.

Co-authored-by: Toshiki Teramura <toshiki.teramura@gmail.com>
@cuviper cuviper mentioned this pull request May 10, 2018
bors bot added a commit that referenced this pull request May 12, 2018
22: Add no_std support r=cuviper a=cuviper

Based on @termoshtt's #15, without the intrusive file splitting.  I also
addressed the other review comments I had left there.  The limitations
of `no_std` are the missing implementations of `Error` and methods based
on `Float`, although `FloatCore` is now used for a few things.  Format
widths are also not supported, as I couldn't find a way to do this
without using a temporary `String`.

Fixes #6.
Closes #15.

Co-authored-by: Josh Stone <cuviper@gmail.com>
@bors bors bot closed this in #22 May 12, 2018
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.

2 participants