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

Tracking issue for std::slice::bytes #27740

Closed
alexcrichton opened this issue Aug 12, 2015 · 12 comments · Fixed by #30187
Closed

Tracking issue for std::slice::bytes #27740

alexcrichton opened this issue Aug 12, 2015 · 12 comments · Fixed by #30187
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

This is a tracking issue for the unstable slice_bytes feature in the standard library. This module has some various helper methods for dealing with blocks of bytes in an efficient fashion, but they seem particularly one-off and not necessarily fitting in with the rest of the design of the standard library.

Some drawbacks are:

  • A one-off trait MutableByteVector is used to give slices the set_memory method.
  • A one-off and oddly placed copy_memory function is used to provide essentially safe bindings to memcpy

Both of these can be implemented in stable Rust otherwise and get optimized down to the same implementation, but it's often useful to call them directly to guarantee the optimization will happen. Needs a decision to whether this functionality should be exposed, where it should be exposed, and how.

@alexcrichton alexcrichton added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. labels Aug 12, 2015
@cesarb
Copy link
Contributor

cesarb commented Aug 13, 2015

I tend to copy that module to every single project I write in Rust (for instance, https://github.com/cesarb/blake2-rfc/blob/master/src/bytes.rs), as a safe and easy-use implementation of memset and memcpy. It's very useful to, for instance, zero the tail of a buffer (memset), or copy from a buffer to another (memcpy). And since I copied them from the standard library, I can be quite certain that the implementation is correct.

They do optimize down to either direct moves/writes through vector registers or to straight calls to memset/memcpy, so I don't see how it being in the standard library would guarantee any optimization over a local copy of the exact same code.

I believe this functionality should be exposed, as it's very useful when dealing with byte (u8) buffers.

The question is how to expose it. The current way works well, but looks strange.

  • For memset, you have to import bytes::MutableByteVector, but it's called x.set_memory(0). The connection between both is not intuitive.
  • For memcpy, on the other hand, you import and use the copy_memory function directly.

This design inconsistency becomes very obvious when you want to use both:

use ...::bytes::{MutableByteVector, copy_memory};

IMHO, either both should be standalone functions, or both should be in a trait:

  • Standalone functions: set_memory(dst: &mut [u8], value: u8), copy_memory(src: &[u8], dst: &mut [u8]) (perhaps flip the parameter order of one of them for consistency).
  • With a trait, we could use a single trait (IMHO this is the cleanest option):
pub trait MutableByteVector {
    fn set_memory(&mut self, value: u8);
    fn copy_memory_from(&mut self, src: &[u8]);

    // We could perhaps even add a memmove here, like:
    fn move_memory(&mut self, dst_offset: usize, src_offset: usize, len: usize);
    // although I don't know how useful that would be
}

Another question is where to expose it. I don't see much point on that single-use module, so either directly on std::slice or on std::mem would be my options.

@cesarb
Copy link
Contributor

cesarb commented Aug 13, 2015

If you want, I could write a RFC for the trait alternative (without memmove).

@aturon
Copy link
Member

aturon commented Nov 4, 2015

Nominating for discussion for 1.6. Discuss post.

cc @briansmith

@alexcrichton
Copy link
Member Author

🔔 This issue is now entering its cycle-long final comment period for deprecation 🔔

@alexcrichton alexcrichton added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed I-nominated labels Nov 5, 2015
@alexcrichton
Copy link
Member Author

The libs team discussed this during triage today and the decision was to deprecate

@SimonSapin
Copy link
Contributor

Is there a recommended, stable replacement for this functionality?

@alexcrichton
Copy link
Member Author

Yes, basically just using iterators and loops.

@SimonSapin
Copy link
Contributor

That’s, hum, unfortunate. Writing a trivial loop like this is annoying enough that it’s tempting (IMO) to use std::ptr::copy_nonoverlapping instead. But it’s unsafe, so there’s always a risk of getting it wrong.

I think this functionality belongs in the standard library, whatever its API looks like.

@alexcrichton
Copy link
Member Author

It's likely this can either be replaced via clone_from_slice or perhaps a new API fill for slices which clones an element into every element of the slice.

@huonw
Copy link
Member

huonw commented Dec 3, 2015

These functions can also exist in a tiny utility crate (where it would make sense to generalise to any Copy type, instead of just u8) until the standard library has appropriate functions, e.g. using specialisation to ensure more general functions have memcpy-performance.

@bluss
Copy link
Member

bluss commented Dec 3, 2015

std::io::Write already provides the same functionality as copy_memory does, using the Write impl for &mut [u8].

@SimonSapin
Copy link
Contributor

@bluss Ah, interesting. The deprecation message should mention that.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Dec 5, 2015
This commit is the standard API stabilization commit for the 1.6 release cycle.
The list of issues and APIs below have all been through their cycle-long FCP and
the libs team decisions are listed below

Stabilized APIs

* `Read::read_exact`
* `ErrorKind::UnexpectedEof` (renamed from `UnexpectedEOF`)
* libcore -- this was a bit of a nuanced stabilization, the crate itself is now
  marked as `#[stable]` and the methods appearing via traits for primitives like
  `char` and `str` are now also marked as stable. Note that the extension traits
  themeselves are marked as unstable as they're imported via the prelude. The
  `try!` macro was also moved from the standard library into libcore to have the
  same interface. Otherwise the functions all have copied stability from the
  standard library now.
* The `#![no_std]` attribute
* `fs::DirBuilder`
* `fs::DirBuilder::new`
* `fs::DirBuilder::recursive`
* `fs::DirBuilder::create`
* `os::unix::fs::DirBuilderExt`
* `os::unix::fs::DirBuilderExt::mode`
* `vec::Drain`
* `vec::Vec::drain`
* `string::Drain`
* `string::String::drain`
* `vec_deque::Drain`
* `vec_deque::VecDeque::drain`
* `collections::hash_map::Drain`
* `collections::hash_map::HashMap::drain`
* `collections::hash_set::Drain`
* `collections::hash_set::HashSet::drain`
* `collections::binary_heap::Drain`
* `collections::binary_heap::BinaryHeap::drain`
* `Vec::extend_from_slice` (renamed from `push_all`)
* `Mutex::get_mut`
* `Mutex::into_inner`
* `RwLock::get_mut`
* `RwLock::into_inner`
* `Iterator::min_by_key` (renamed from `min_by`)
* `Iterator::max_by_key` (renamed from `max_by`)

Deprecated APIs

* `ErrorKind::UnexpectedEOF` (renamed to `UnexpectedEof`)
* `OsString::from_bytes`
* `OsStr::to_cstring`
* `OsStr::to_bytes`
* `fs::walk_dir` and `fs::WalkDir`
* `path::Components::peek`
* `slice::bytes::MutableByteVector`
* `slice::bytes::copy_memory`
* `Vec::push_all` (renamed to `extend_from_slice`)
* `Duration::span`
* `IpAddr`
* `SocketAddr::ip`
* `Read::tee`
* `io::Tee`
* `Write::broadcast`
* `io::Broadcast`
* `Iterator::min_by` (renamed to `min_by_key`)
* `Iterator::max_by` (renamed to `max_by_key`)
* `net::lookup_addr`

New APIs (still unstable)

* `<[T]>::sort_by_key` (added to mirror `min_by_key`)

Closes rust-lang#27585
Closes rust-lang#27704
Closes rust-lang#27707
Closes rust-lang#27710
Closes rust-lang#27711
Closes rust-lang#27727
Closes rust-lang#27740
Closes rust-lang#27744
Closes rust-lang#27799
Closes rust-lang#27801
cc rust-lang#27801 (doesn't close as `Chars` is still unstable)
Closes rust-lang#28968
bors added a commit that referenced this issue Dec 6, 2015
This commit is the standard API stabilization commit for the 1.6 release cycle.
The list of issues and APIs below have all been through their cycle-long FCP and
the libs team decisions are listed below

Stabilized APIs

* `Read::read_exact`
* `ErrorKind::UnexpectedEof` (renamed from `UnexpectedEOF`)
* libcore -- this was a bit of a nuanced stabilization, the crate itself is now
  marked as `#[stable]` and the methods appearing via traits for primitives like
  `char` and `str` are now also marked as stable. Note that the extension traits
  themeselves are marked as unstable as they're imported via the prelude. The
  `try!` macro was also moved from the standard library into libcore to have the
  same interface. Otherwise the functions all have copied stability from the
  standard library now.
* `fs::DirBuilder`
* `fs::DirBuilder::new`
* `fs::DirBuilder::recursive`
* `fs::DirBuilder::create`
* `os::unix::fs::DirBuilderExt`
* `os::unix::fs::DirBuilderExt::mode`
* `vec::Drain`
* `vec::Vec::drain`
* `string::Drain`
* `string::String::drain`
* `vec_deque::Drain`
* `vec_deque::VecDeque::drain`
* `collections::hash_map::Drain`
* `collections::hash_map::HashMap::drain`
* `collections::hash_set::Drain`
* `collections::hash_set::HashSet::drain`
* `collections::binary_heap::Drain`
* `collections::binary_heap::BinaryHeap::drain`
* `Vec::extend_from_slice` (renamed from `push_all`)
* `Mutex::get_mut`
* `Mutex::into_inner`
* `RwLock::get_mut`
* `RwLock::into_inner`
* `Iterator::min_by_key` (renamed from `min_by`)
* `Iterator::max_by_key` (renamed from `max_by`)

Deprecated APIs

* `ErrorKind::UnexpectedEOF` (renamed to `UnexpectedEof`)
* `OsString::from_bytes`
* `OsStr::to_cstring`
* `OsStr::to_bytes`
* `fs::walk_dir` and `fs::WalkDir`
* `path::Components::peek`
* `slice::bytes::MutableByteVector`
* `slice::bytes::copy_memory`
* `Vec::push_all` (renamed to `extend_from_slice`)
* `Duration::span`
* `IpAddr`
* `SocketAddr::ip`
* `Read::tee`
* `io::Tee`
* `Write::broadcast`
* `io::Broadcast`
* `Iterator::min_by` (renamed to `min_by_key`)
* `Iterator::max_by` (renamed to `max_by_key`)
* `net::lookup_addr`

New APIs (still unstable)

* `<[T]>::sort_by_key` (added to mirror `min_by_key`)

Closes #27585
Closes #27704
Closes #27707
Closes #27710
Closes #27711
Closes #27727
Closes #27740
Closes #27744
Closes #27799
Closes #27801
cc #27801 (doesn't close as `Chars` is still unstable)
Closes #28968
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants