-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add support for Haiku x86 and x86_64 #407
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
Conversation
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Looks like travis is also failing when running the style script.
pub fn getifaddrs(ifap: *mut *mut ifaddrs) -> ::c_int; | ||
#[cfg(not(target_os = "haiku"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these functions get pushed down into the lower modules instead of adding #[cfg]
here?
pub type c_long = i64; | ||
#[cfg(target_pointer_width = "32")] | ||
pub type c_ulong = u32; | ||
#[cfg(target_pointer_width = "64")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these #[cfg]
get pushed down farther instead of being here? (e.g. a b32 and b64 module)
@@ -0,0 +1,719 @@ | |||
use dox::mem; | |||
|
|||
cfg_if! { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be pushed down to separate modules? Currently we try to keep each module to have at most one invocation of cfg_if!
for delegating to sub-modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really follow the logic there.. If I move all the ifcfg's into something like this:
mod.rs:
use dox::mem;
cfg_if! {
if #[cfg(target_pointer_width = "64")] {
mod b64;
} else {
mod b32;
}
}
b32.rs:
pub type c_long = i32;
pub type c_ulong = u32;
const ULONG_SIZE: usize = 32;
Then I begin to get additional style issues:
src/unix/haiku/mod.rs:11 - typedef found after module when it belongs before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, ok. Move it to the end.
Two platforms encountered an error. It sucks having to touch so much unrelated code to add a platform in :-| arm-linux-androideabi
x86_64-unknown-linux-gnu
|
Ah the arguments there likely just need to become |
Looks like there's still some CI failures? |
Yeah, same issues. Adding the ::'s didn't have any change. |
Oh this was actually in a block that should go away in the sense that these functions don't exist on Android, so you can just push it farther down into the linux-only module. |
Might want to squash these if accepted :-) |
Looks good to me, want to squash? |
Github has a new automatic squash.. any issues trying it out? :-D |
Gotcha. I'll squash and resubmit. I took a crack at bootstrapping and ran into an interesting compiler-rt error: llvm supports Haiku, so not really sure what's going on there... the error doesn't make sense either. |
No description provided.