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

Refactor stdlib for pal #39325

Closed
wants to merge 4 commits into from
Closed

Conversation

cassiersg
Copy link

This is a small step towards pal. I moved all platform-specific code from libstd/{path,f32,f64}.rs to libstd/sys.
For the float implementation, I moved only the parts that are currently platform-specific, which leaves direct dependency on libc in libstd/f32.rs and libstd/f64.rs. I don't know if it is better to move all the dependency to libc in sys.

This removes one exception in the tidy lint 'pal'.
This removes exceptions in the tidy lint 'pal'.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (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.

@alexcrichton
Copy link
Member

r? @brson

@rust-highfive rust-highfive assigned brson and unassigned alexcrichton Jan 26, 2017
@brson
Copy link
Contributor

brson commented Jan 26, 2017

@bors r+ beautiful thanks @cassiersg !

@bors
Copy link
Contributor

bors commented Jan 26, 2017

📌 Commit 662fef6 has been approved by brson

@brson
Copy link
Contributor

brson commented Jan 26, 2017

@bors r-

@brson
Copy link
Contributor

brson commented Jan 26, 2017

@cassiersg the float commit looks incomplete to me - it adds the sys definitions but does not rewrite the float modules to use the sys/float modules. I'd expect std/f32 and std/f64 to change and for those files to be removed from the pal whitelist. Can you add another commit to do that?

Also remove exceptions in tidy lint 'pal'.
@cassiersg
Copy link
Author

@brson I actually forgot to commit some files. Everything should be there now.

@brson
Copy link
Contributor

brson commented Jan 27, 2017

@bors r+

Thanks @cassiersg. As a follow up to this (or in this PR if you want), even the common C functions need to be moved into 'sys' - a platform-independent std is ultimately not allowed to call C directly. Duplication of the C declarations is fine.

@bors
Copy link
Contributor

bors commented Jan 27, 2017

📌 Commit badd513 has been approved by brson

@bors
Copy link
Contributor

bors commented Jan 27, 2017

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Jan 27, 2017

📌 Commit badd513 has been approved by brson

@alexcrichton
Copy link
Member

I'm getting some failures on the rollup (#39353) on 64-bit MSVC:

   Doc-tests std

running 2 tests
thread 'rustc' panicked at 'test executable failed:

thread 'main' panicked at 'assertion failed: abs_difference <= f32::EPSILON', <anon>:12
note: Run with `RUST_BACKTRACE=1` for a backtrace.
', src\librustdoc\test.rs:305
note: Run with `RUST_BACKTRACE=1` for a backtrace.
test f32::f32::hypot_0 ... FAILED
test f64::f64::hypot_0 ... ok

failures:

failures:
    f32::f32::hypot_0

test result: FAILED. 1 passed; 1 failed; 0 ignored; 0 measured

error: test failed

I think those may be related to this PR? (haven't dug deeply though)

bors added a commit that referenced this pull request Jan 28, 2017
@bors
Copy link
Contributor

bors commented Jan 28, 2017

⌛ Testing commit badd513 with merge 1881594...

@bors
Copy link
Contributor

bors commented Jan 28, 2017

💔 Test failed - status-appveyor

@brson
Copy link
Contributor

brson commented Feb 7, 2017

@cassiersg Can you peek at the errors here?

@cassiersg
Copy link
Author

@brson I don't have currently access to a windows system. I'll look at this next weekend.

#[cfg_attr(target_env = "msvc", link_name = "__lgamma_r")]
pub fn lgammaf_r(n: c_float, sign: &mut c_int) -> c_float;

#[cfg_attr(target_env = "msvc", link_name = "_hypot")]
Copy link

Choose a reason for hiding this comment

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

I think the link_names here should be __lgammaf_r and _hypotf.

Since the test failed on f32::hypot_0 could this have caused the failure?

@cassiersg
Copy link
Author

@brson I have not yet access to a windows environment. I just pushed a fix for the bug @whataloadofwhat found, since it is probably the cause of test failure.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Feb 13, 2017
@alexcrichton
Copy link
Member

@bors: r=brson

@bors
Copy link
Contributor

bors commented Feb 14, 2017

📌 Commit 7298535 has been approved by brson

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 14, 2017
Refactor stdlib for pal

This is a small step towards [pal](https://internals.rust-lang.org/t/refactoring-std-for-ultimate-portability/4301/40). I moved all platform-specific code from `libstd/{path,f32,f64}.rs` to `libstd/sys`.
For the float implementation, I moved only the parts that are currently platform-specific, which leaves direct dependency on libc in `libstd/f32.rs` and `libstd/f64.rs`. I don't know if it is better to move all the dependency to libc in  `sys`.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 14, 2017
Refactor stdlib for pal

This is a small step towards [pal](https://internals.rust-lang.org/t/refactoring-std-for-ultimate-portability/4301/40). I moved all platform-specific code from `libstd/{path,f32,f64}.rs` to `libstd/sys`.
For the float implementation, I moved only the parts that are currently platform-specific, which leaves direct dependency on libc in `libstd/f32.rs` and `libstd/f64.rs`. I don't know if it is better to move all the dependency to libc in  `sys`.
@bors
Copy link
Contributor

bors commented Feb 14, 2017

⌛ Testing commit 7298535 with merge ad597e7...

@bors
Copy link
Contributor

bors commented Feb 14, 2017

💔 Test failed - status-travis

@brson
Copy link
Contributor

brson commented Mar 9, 2017

Errors are legit @cassiersg.

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to resubmit with tests fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants