Add a crapton of missing trait impls and docs by Gankra · Pull Request #40 · mozilla/thin-vec

Gankra · 2022-12-05T18:33:36Z

This is almost entirely copy-pasting from std. I also fleshed out a bunch of docs in Drain. This is a yakshave to prep for adding splice support.

Gankra · 2022-12-05T18:33:46Z

r? @nnethercote

This is almost entirely copy-pasting from std. I also fleshed out a bunch of docs in Drain. This is a yakshave to prep for adding splice support.

Gankra · 2022-12-05T19:36:36Z

bonus: actually handling isize::MAX stuff

nnethercote · 2022-12-05T21:17:37Z

src/lib.rs

-
 // Utils for computing layouts of allocations

 fn alloc_size<T>(cap: usize) -> usize {


Add a doc comment to alloc_size explaining that it can panic?

nnethercote · 2022-12-05T21:19:14Z

src/lib.rs

+    /// If it is important to know the exact allocated capacity of a `ThinVec`,
+    /// always use the [`capacity`] method after construction.
+    ///
+    /// **NOTE**: unlike Vec, ThinVec **MUST** allocate to keep track of non-zero


Nit: Vec and ThinVec here don't have backticks. I'm not sure how hard you're trying to get every one.

nnethercote · 2022-12-05T21:20:43Z

src/lib.rs

+    /// If it is important to know the exact allocated capacity of a `ThinVec`,
+    /// always use the [`capacity`] method after construction.
+    ///
+    /// **NOTE**: unlike Vec, ThinVec **MUST** allocate to keep track of non-zero


Suggested change

/// **NOTE**: unlike Vec, ThinVec **MUST** allocate to keep track of non-zero

/// **NOTE**: unlike Vec, ThinVec **MUST** allocate once to keep track of non-zero

nnethercote · 2022-12-05T21:22:33Z

src/lib.rs

+    ///
+    /// let mut vec = thin_vec![thin_vec![1, 0, 0],
+    ///                    thin_vec![0, 1, 0],
+    ///                    thin_vec![0, 0, 1]];


Formatting on this line could be nicer.

This is the formatting std had shrug

nnethercote · 2022-12-05T21:24:47Z

src/lib.rs

+    /// Convert a boxed slice into a vector by transferring ownership of
+    /// the existing heap allocation.
+    ///
+    /// **NOTE:** unlike std, this must reallocate to change the layout!


std? (i.e. with backticks)

nnethercote · 2022-12-05T21:25:04Z

src/lib.rs

+}
+
+impl<T> From<Vec<T>> for ThinVec<T> {
+    /// Convert a std::Vec into a ThinVec.


More cases below, I won't point them all out, a bit of grepping will find them if you care to change them.

nnethercote · 2022-12-05T21:26:44Z

src/lib.rs

+    /// assert_eq!(ThinVec::from(b), thin_vec![1, 2, 3]);
+    /// ```
+    fn from(s: Vec<T>) -> Self {
+        // Can just lean on the fact that `Box<[T]>` -> `Vec<T>` is Free.


Why is Box<[T]> relevant here?

nnethercote · 2022-12-05T21:27:08Z

src/lib.rs

+    /// assert_eq!(Vec::from(b), vec![1, 2, 3]);
+    /// ```
+    fn from(s: ThinVec<T>) -> Self {
+        // Can just lean on the fact that `Box<[T]>` -> `Vec<T>` is Free.


nnethercote · 2022-12-05T21:31:09Z

src/lib.rs

+    /// It's ok to use IterMut here because it promises to only take mutable
+    /// refs to the parts we haven't yielded yet.
+    ///
+    /// A downside of this (and the *mut below is that it makes this Invariant, when


This sentence reads strangely. Are the parentheses in the wrong place? Also, "Inductive" doesn't need to be capitalized?

nnethercote · 2022-12-05T21:31:32Z

src/lib.rs

+    /// completely done with the IterMut in the `drop` impl of this type (or miri will get mad).
+    ///
+    /// Since we set the `len` of this to be before `IterMut`, we can use that `len`
+    /// to retrieve the index of the start of the drain range later.


Nice comment! I think you enjoyed writing that one :)

nnethercote · 2022-12-05T21:32:52Z

src/lib.rs

+    #[must_use]
+    pub fn as_slice(&self) -> &[T] {
+        // SAFETY: this is A-OK because the elements that the underlying
+        // iterator still points at are still logically initialized and continguous.


Suggested change

// iterator still points at are still logically initialized and continguous.

// iterator still points at are still logically initialized and contiguous.

unless "continguous" is a new word The Kids Are Using These Days.

nnethercote

Looks good. A few minor suggestions. I admit I didn't read all the example code in the doc comments, though I did read the comment prose and the code in the tests.

nnethercote · 2022-12-05T21:39:47Z

Note: there's a clippy warning that might be worth using an attribute to placate. GitHub wouldn't let me comment on the specific line for some reason.

eggyal · 2023-02-19T21:26:37Z

src/lib.rs

+    let cap = cap as isize;
+    let header_size = mem::size_of::<Header>() as isize;
+    let elem_size = mem::size_of::<T>() as isize;
+    let padding = padding::<T>() as isize;


Apologies for dropping in belatedly, but I was just reviewing changes between v0.2.9 and v0.2.11 (for rust-lang/rust#108245) and spotted this...

Isn't there a risk that these casts (okay, less so the latter three but certainly the cap one) might silently overflow isize::MAX and result in a negative isize value? Better to use TryInto, I think?

...yikes! good catch.

#45

Add a crapton of missing trait impls and docs

35ed253

This is almost entirely copy-pasting from std. I also fleshed out a bunch of docs in Drain. This is a yakshave to prep for adding splice support.

Gankra force-pushed the backlog branch from fdea8cc to 887455c Compare December 5, 2022 19:31

properly handle isize::MAX overflow

3dcaf54

Gankra force-pushed the backlog branch from 887455c to 3dcaf54 Compare December 5, 2022 19:33

Gankra mentioned this pull request Dec 5, 2022

Add splice to thin_vec #41

Merged

nnethercote reviewed Dec 5, 2022

View reviewed changes

nnethercote approved these changes Dec 5, 2022

View reviewed changes

Apply doc fixes as requested

c1e68ae

Gankra merged commit 9e18364 into main Dec 5, 2022

eggyal reviewed Feb 19, 2023

View reviewed changes


		// Utils for computing layouts of allocations

		fn alloc_size<T>(cap: usize) -> usize {

	/// NOTE: unlike Vec, ThinVec MUST allocate to keep track of non-zero
	/// NOTE: unlike Vec, ThinVec MUST allocate once to keep track of non-zero

	// iterator still points at are still logically initialized and continguous.
	// iterator still points at are still logically initialized and contiguous.

Conversation

Gankra commented Dec 5, 2022

Uh oh!

Gankra commented Dec 5, 2022

Uh oh!

Gankra commented Dec 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nnethercote left a comment

Choose a reason for hiding this comment

Uh oh!

nnethercote commented Dec 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants