Skip to content
This repository was archived by the owner on Jan 24, 2022. It is now read-only.

Remove redundant and incorrect target FPU check #185

Merged
merged 1 commit into from
Mar 20, 2019
Merged

Remove redundant and incorrect target FPU check #185

merged 1 commit into from
Mar 20, 2019

Conversation

jonas-schievink
Copy link
Contributor

Most target triples with FPU end in gnueabihf or musleabihf, so this isn't catching them. Since the has_fpu function does, this shouldn't change behavior.

Most target triples with FPU end in `gnueabihf` or `musleabihf`, so this isn't catching them. Since the `has_fpu` function does, this shouldn't change behavior.
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @therealprof (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@thejpster
Copy link
Contributor

I'm a little confused by this. I thought cortex-m-rt only supported bare-metal no_std thumbv[678]e?m-none-eabi(hf)? So it doesn't matter if it can't parse gnueabi and musl targets?

@jonas-schievink
Copy link
Contributor Author

Yes, that's true, sorry for the confusion. There might be more targets added in the future which use a different ABI, but for now this just removes redundant code.

@thejpster
Copy link
Contributor

How else would the 'has_fpu' feature get set, which is used in src/lib.rs to gate whether to include the FPU-enable code?

@jonas-schievink
Copy link
Contributor Author

There's a has_fpu function futher down in the build script. It's called at the start of the build script.

@thejpster
Copy link
Contributor

Oh yes. I was confused by the two identical pieces of code, and how "View File" in Github opens the file in your branch (i.e. post-changes).

@thejpster
Copy link
Contributor

@bors r+

Copy link
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@therealprof
Copy link
Contributor

bors r+

bors bot added a commit that referenced this pull request Mar 20, 2019
185: Remove redundant and incorrect target FPU check r=therealprof a=jonas-schievink

Most target triples with FPU end in `gnueabihf` or `musleabihf`, so this isn't catching them. Since the `has_fpu` function does, this shouldn't change behavior.

Co-authored-by: Jonas Schievink <[email protected]>
@bors
Copy link
Contributor

bors bot commented Mar 20, 2019

Build succeeded

@bors bors bot merged commit f491d14 into rust-embedded:master Mar 20, 2019
@jonas-schievink jonas-schievink deleted the patch-3 branch March 20, 2019 13:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants