library: use core::ffi::c_* types - #159868
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Some changes occurred in cc @Amanieu, @folkertdev, @sayantn
cc @tgross35 |
|
r? @nia-e rustbot has assigned @nia-e. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
r? @tgross35 |
|
|
There was a problem hiding this comment.
Most of this seems completely reasonable. However, I don't think we should change imports from std to core in examples. E.g.:
+ /// use core::ffi::c_char;
/// use std::ffi::CString;
- /// use std::os::raw::c_char;It's a bit cleaner to import only from std::ffi than to mix core and std.
|
Reminder, once the PR becomes ready for a review, use |
| /// use std::ffi::{CString, CStr}; | ||
| /// use core::ffi::CStr; | ||
| /// use std::ffi::CString; |
There was a problem hiding this comment.
I'm greatly in favor of using std::ffi::* instead of std::os::raw::*, but I'm not convinced that using core in documentation is the best idea? I'd prefer to guide users towards using the std re-exports.
Especially because core isn't available in the 2015 edition, so users copying the code into a sample file and running bare rustc on it will get an error. (Somewhat mitigated by rust-lang/compiler-team#1019, but still).
There was a problem hiding this comment.
Especially because
coreisn't available in the 2015 edition, so users copying the code into a sample file and running barerustcon it will get an error.
Is this really a significant issue? If someone compiles the example with the 2015 edition, the compiler already shows a suggested fix:
error[E0433]: cannot find `core` in the crate root
--> <source>:1:5
|
1 | use core::ffi::CStr;
| ^^^^ you might be missing crate `core`
|
help: try using `std` instead of `core`
|
1 - use core::ffi::CStr;
1 + use std::ffi::CStr;
|
error: aborting due to 1 previous error
For more information about this error, try `rustc --explain E0433`.
There was a problem hiding this comment.
Ditto on that #159868 (review). It's also just consistency; if you have std available it's more common to import everything from std than to split it up.
There was a problem hiding this comment.
I think it would be reasonable to customize the resolve error under 2015 to mention the difference in a note.
|
☔ The latest upstream changes (presumably #160517) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
No description provided.