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], embedded support #246

Merged
merged 4 commits into from
Apr 18, 2020
Merged

Conversation

PTaylor-us
Copy link
Contributor

@PTaylor-us PTaylor-us commented Apr 18, 2020

Fixes #243
Requires jhpratt/standback#6

This should allow time to be used with no_std targets assuming the author of standback accepts my PR.

The second commit isn't required, but (in my opinion) is a cleaner way of checking the std feature. It might be completely wrong.

@PTaylor-us PTaylor-us changed the title no_std support (Fixes #243, Requires jhpratt/standback#6) no_std support Apr 18, 2020
@PTaylor-us
Copy link
Contributor Author

Waiting for resolution of jhpratt/standback#6 and version bump.

@jhpratt
Copy link
Member

jhpratt commented Apr 18, 2020

Standback is my crate, FYI. Consider that done after a quick review later.

The second commit is correct in functionality, but I felt it was cleaner to emit two separate flags. I'm open to being convinced, though!

As with standback, I'll handle the CI side of things to avoid unnecessary duplication.

@PTaylor-us
Copy link
Contributor Author

Standback is my crate, FYI.

Boy do I feel silly 😳.

The second commit is correct in functionality, but I felt it was cleaner to emit two separate flags. I'm open to being convinced, though!

As I've mentioned, I'm pretty new. I think I've seen it as in my commit more, but don't have strong feelings about it. I think for me, it doesn't hide the no_std flag. When I see cfg(std), I know where that std configuration is coming from just by looking at the Cargo.toml. Just my two cents. Thank you for confirming that it's functionally correct.

@jhpratt
Copy link
Member

jhpratt commented Apr 18, 2020

Boy do I feel silly 😳.

No worries! Unless you were looking at who has control on crates.io or who has committed to the repos, there's no reason for you to know. And realistically, it doesn't much matter.

As I've mentioned, I'm pretty new. I think I've seen it as in my commit more, but don't have strong feelings about it. I think for me, it doesn't hide the no_std flag. When I see cfg(std), I know where that std configuration is coming from just by looking at the Cargo.toml. Just my two cents. Thank you for confirming that it's functionally correct.

For what it's worth if it's clearer to new people, that's typically what's preferred! cfg aliases certainly isn't something a beginner needs to be familiar with, either.

Make embedded its own check, rather than part of the OS-level checking.
Also remove nightly, as we already know it works on stable.
@jhpratt
Copy link
Member

jhpratt commented Apr 18, 2020

Looks like everything is working as expected. All I really did was get CI to a place I like.

Thank you!

@jhpratt jhpratt changed the title no_std support #![no_std], embedded support Apr 18, 2020
@jhpratt jhpratt merged commit a933f2a into time-rs:master Apr 18, 2020
@PTaylor-us PTaylor-us deleted the no_std-support branch April 19, 2020 14:58
@jhpratt jhpratt added C-bug Category: bug in current code C-feature-request Category: a new feature (not already implemented) labels Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug in current code C-feature-request Category: a new feature (not already implemented)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't build with no_std because of cfg-if dependency
2 participants