Split qnx target based on target env #4615
Conversation
AkhilTThomas
force-pushed
the
feat/qnx8
branch
3 times, most recently
from
adb14ed to
991ccb6
Compare
|
Pinging @jonathanpallant @flba-eb Could you take a look at this? |
- Created nto7071 and nto80 modules. - Merged neutrino.rs into parent module.
AkhilTThomas
force-pushed
the
feat/qnx8
branch
from
c8a47d9 to
584df13
Compare
|
@tgross35 Could you take a look at this? one of the target maintainers has approved it. |
There was a problem hiding this comment.
- Created nto7x and nto8x modules.
This part seems reasonable
- Merged neutrino.rs into parent module.
Does neutrino.rs map to a neutrino.h? If so, I would prefer to keep this separate since we are migrating other targets to something similar, i.e. mapping one .h file to one .rs file rather than keeping everything together. If there are version-specific aspects, these parts can go in {nto7x,nto8x}/neutrino.h
- nto7x/mod.rs is identical to the original mod.rs with only difference being the crate import adjustments.
- nto8x/mod.rs is same as nto7x/mod.rs (placeholder till the actual 8.0 support PR)
I don't understand the reasoning behind this part of the change: typically we keep code that is common across all versions in a parent module, and then only parts that are different move into version-specific modules. For FreeBSD the version-specific files are only a few hundred lines. Having an identical or nearly-identical 3k line file here isn't very nice.
| cfg_if! { | ||
| if #[cfg(target_arch = "x86_64")] { | ||
| pub use crate::unix::nto::x86_64::*; | ||
| } else if #[cfg(target_arch = "aarch64")] { | ||
| use crate::unix::nto::aarch64::*; | ||
| } else { | ||
| panic!("Unsupported arch"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Why is the first branch pub use but the second is use?
Also, typically libc just exports nothing if the architecture isn't supported, rather than having a compiler error. If this is kept, it should be compiler_error! rather than panic! , and it would be good to add "for QNX Neutrino" to the message (since presumably we support the architecture otherwise).
| cfg_if! { | ||
| if #[cfg(target_arch = "x86_64")] { | ||
| use crate::unix::nto::x86_64::*; | ||
| } else if #[cfg(target_arch = "aarch64")] { | ||
| use crate::unix::nto::aarch64::*; | ||
| } else { | ||
| panic!("Unsupported arch"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Same questions as for nto7x
|
@AkhilTThomas I'm looking forward to see that merged :). Did you already have the time to fix the comments. |
@FScholPer Not really. Bit busy these days. I will get back to this next week |
Description
This PR is a preparation to implement the QNX 8.0 support by splitting the implementation logic into separate modules.
It's similar to the approach used by
freebsdto support multiple versions which have a different "C" function / structure / constants.nto70,nto71andnto71_iosock.nto80is currently identical to thento7071implementation to keep the files diff-able.Sources
Checklist
libc-test/semverhave been updated*LASTor*MAXareincluded (see #3131)
cd libc-test && cargo test --target mytarget);especially relevant for platforms that may not be checked in CI