-
Notifications
You must be signed in to change notification settings - Fork 12.2k
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
DOC: Add FFI example for slice::from_raw_parts() #123374
Conversation
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 making a PR! Agreed it'd be good to have guidance like this in the docs 👍
/// dbg!(data); | ||
/// // ... | ||
/// } | ||
/// ``` |
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.
A couple of requests for the example:
- When there's unsafe functions or blocks in the documentation, please put
# Safety
header and// SAFETY
comments so the docs are a good example of what we'd like people to do. dbg!
inside rustdoc examples isn't particularly useful, especially for a function that's never called. Could you maybe do some simple computation on it and return it, so that it could be called andassert!
ed instead? That way Miri can check that it's not accidentally UB, too.
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 review!
I have added the "safety" comments, please let me know if I should make any changes to the text.
I also changed the function to return a value and added an assert.
I'm not sure if that makes sense, though, since this function would normally be called from C++ or some other FFI language.
Alternatively, I could also provide a more general (and more useful) function:
unsafe fn ffi_slice<'a, T>(ptr: *const T, len: usize) -> &'a [T] {
if ptr.is_null() {
// NB: `len` is assumed to be 0.
&[]
} else {
unsafe { std::slice::from_raw_parts(ptr, len) }
}
}
I have actually used that function in my code: https://github.com/AudioSceneDescriptionFormat/asdfspline-rust/blob/e2608b07e2749caf71e43f23d4592849d27dd450/ffi/src/lib.rs#L59-L89.
However, this is a bit more complicated due to the lifetime annotation, so it might not be appropriate here?
This comment has been minimized.
This comment has been minimized.
Thanks, this addresses my concerns. I agree a caller from C++ would be more realistic, but there's not really a good way to put that in a doctest, so it's fine. The point is just to make sure that it runs, giving an opportunity for the runtime UB checks to double-check that everything is fine. (Which I do think it is, but for @bors r+ rollup |
…=scottmcm DOC: Add FFI example for slice::from_raw_parts() For some discussion, see https://users.rust-lang.org/t/missing-guidance-on-converting-ffi-ptr-length-to-slice/106048 See also rust-lang#120608.
Rollup of 6 pull requests Successful merges: - rust-lang#123374 (DOC: Add FFI example for slice::from_raw_parts()) - rust-lang#126127 (Spell out other trait diagnostic) - rust-lang#126228 (Provide correct parent for nested anon const) - rust-lang#126249 (Simplify `[T; N]::try_map` signature) - rust-lang#126256 (Add {{target}} substitution to compiletest) - rust-lang#126263 (Make issue-122805.rs big endian compatible) r? `@ghost` `@rustbot` modify labels: rollup
…=scottmcm DOC: Add FFI example for slice::from_raw_parts() For some discussion, see https://users.rust-lang.org/t/missing-guidance-on-converting-ffi-ptr-length-to-slice/106048 See also rust-lang#120608.
…kingjubilee Rollup of 8 pull requests Successful merges: - rust-lang#123374 (DOC: Add FFI example for slice::from_raw_parts()) - rust-lang#126210 (docs(core): make more const_ptr doctests assert instead of printing) - rust-lang#126228 (Provide correct parent for nested anon const) - rust-lang#126242 (Simplify provider api to improve llvm ir) - rust-lang#126249 (Simplify `[T; N]::try_map` signature) - rust-lang#126256 (Add {{target}} substitution to compiletest) - rust-lang#126263 (Make issue-122805.rs big endian compatible) - rust-lang#126286 (Make `storage-live.rs` robust against rustc internal changes.) r? `@ghost` `@rustbot` modify labels: rollup
…=scottmcm DOC: Add FFI example for slice::from_raw_parts() For some discussion, see https://users.rust-lang.org/t/missing-guidance-on-converting-ffi-ptr-length-to-slice/106048 See also rust-lang#120608.
…kingjubilee Rollup of 16 pull requests Successful merges: - rust-lang#123374 (DOC: Add FFI example for slice::from_raw_parts()) - rust-lang#124514 (Recommend to never display zero disambiguators when demangling v0 symbols) - rust-lang#125978 (Cleanup: HIR ty lowering: Consolidate the places that do assoc item probing & access checking) - rust-lang#125980 (Nvptx remove direct passmode) - rust-lang#126187 (For E0277 suggest adding `Result` return type for function when using QuestionMark `?` in the body.) - rust-lang#126210 (docs(core): make more const_ptr doctests assert instead of printing) - rust-lang#126249 (Simplify `[T; N]::try_map` signature) - rust-lang#126256 (Add {{target}} substitution to compiletest) - rust-lang#126263 (Make issue-122805.rs big endian compatible) - rust-lang#126281 (set_env: State the conclusion upfront) - rust-lang#126286 (Make `storage-live.rs` robust against rustc internal changes.) - rust-lang#126287 (Update a cranelift patch file for formatting changes.) - rust-lang#126301 (Use `tidy` to sort crate attributes for all compiler crates.) - rust-lang#126305 (Make PathBuf less Ok with adding UTF-16 then `into_string`) - rust-lang#126310 (Migrate run make prefer rlib) - rust-lang#126314 (fix RELEASES: we do not support upcasting to auto traits) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#123374 - mgeier:doc-slice-from-raw-parts, r=scottmcm DOC: Add FFI example for slice::from_raw_parts() For some discussion, see https://users.rust-lang.org/t/missing-guidance-on-converting-ffi-ptr-length-to-slice/106048 See also rust-lang#120608.
…kingjubilee Rollup of 16 pull requests Successful merges: - rust-lang#123374 (DOC: Add FFI example for slice::from_raw_parts()) - rust-lang#124514 (Recommend to never display zero disambiguators when demangling v0 symbols) - rust-lang#125978 (Cleanup: HIR ty lowering: Consolidate the places that do assoc item probing & access checking) - rust-lang#125980 (Nvptx remove direct passmode) - rust-lang#126187 (For E0277 suggest adding `Result` return type for function when using QuestionMark `?` in the body.) - rust-lang#126210 (docs(core): make more const_ptr doctests assert instead of printing) - rust-lang#126249 (Simplify `[T; N]::try_map` signature) - rust-lang#126256 (Add {{target}} substitution to compiletest) - rust-lang#126263 (Make issue-122805.rs big endian compatible) - rust-lang#126281 (set_env: State the conclusion upfront) - rust-lang#126286 (Make `storage-live.rs` robust against rustc internal changes.) - rust-lang#126287 (Update a cranelift patch file for formatting changes.) - rust-lang#126301 (Use `tidy` to sort crate attributes for all compiler crates.) - rust-lang#126305 (Make PathBuf less Ok with adding UTF-16 then `into_string`) - rust-lang#126310 (Migrate run make prefer rlib) - rust-lang#126314 (fix RELEASES: we do not support upcasting to auto traits) r? `@ghost` `@rustbot` modify labels: rollup
For some discussion, see https://users.rust-lang.org/t/missing-guidance-on-converting-ffi-ptr-length-to-slice/106048
See also #120608.