Add support for in-place map for `Vec`s of types with same size (take 3) #16853

tbu- · 2014-08-29T19:56:47Z

This is implemented using a new struct PartialVec which implements the proper
drop semantics in case the conversion is interrupted by an unwind.

For the old pull requests, see #15302, #16369.

huonw · 2014-09-05T12:32:37Z

src/libcollections/vec.rs

+
+        let start = vec.as_mut_ptr();
+
+        // This `as int` cast is safe, because the size of the elements of the


This justification isn't actually required, since this is how offset is designed to work. :)

How is offset designed to work?

Actually I'm not even sure what I'm doing is correct, because the docs on ptr::offset say that the given offset must be inbound, which negative ones technically aren't.

Oh, really? So the following is technically undefined?

let p = some_vec.as_ptr(); let q = p.offset(10); let r = q.offset(-10);

However, it doesn't particularly matter since offset is widely used with the output of len(); this is not an unusual case (e.g. some_slice.iter() is created with offset and len).

@huonw: No, that q.offset(-10) is perfectly well defined if p.offset(10) was correct. It's within the bounds of the object it was based on.

huonw · 2014-09-05T12:55:30Z

cc @aturon

(Sorry that this sat unnoticed for so long @tbu-!)

thestinger · 2014-09-05T13:16:29Z

I think this is a lot more complicated than it needs to be, even with failure safety in mind.

tbu- · 2014-09-05T13:50:50Z

@thestinger What specifically? I had the impression that I did pretty much the minimum required to get this running.

aturon · 2014-09-08T16:03:14Z

src/libcollections/vec.rs

+    ///
+    /// Removes the next `T` from the vector and returns it as `Some(T)`, or
+    /// `None` if there are none left.
+    fn pop(&mut self) -> Option<T> {


Is this intentionally private?

No, this should be public.

aturon · 2014-09-08T16:05:04Z

@tbu- Can you elaborate on the use case you have in mind here?

I'm guessing the intent is just to support the map_inplace function. I'm wondering whether that could be the sole public interface, rather than exposing PartialVec? (My understanding is that the machinery here is needed to properly handle unwinding.) Or are there use cases you have in mind that go beyond map_inplace?

aturon · 2014-09-08T16:15:00Z

To elaborate slightly, it seems like the interface here effectively forces you to do something like your map_inplace, although technically you can pop off multiple elements before pushing in their replacements.

If PartialVec were private, I wonder whether the various methods it supplies could instead be moved directly into map_inplace? Basically, use PartialVec as a temporary place to move the Vec into, with the right drop semantics, but otherwise perform all of the work directly in map_inplace. That seems like it could simplify the code.

aturon · 2014-09-08T16:15:49Z

src/libcollections/vec.rs

+    /// ]);
+    /// assert_eq!(u8s.as_slice(), &[[0x11, 0x22], [0x33, 0x44]]);
+    /// ```
+    pub fn map_inplace<U>(self, f: |T| -> U) -> Vec<U> {


Minor detail, but why not map_in_place?

I don't know, it felt more natural to me, maybe because map_in_place sounds less like a hyphen between "in" and "place". But I have absolutely no problem in renaming this if you want it.

tbu- · 2014-09-08T17:13:40Z

About the public interface: I don't know anymore why the PartialVec was exposed, but you're probably right that it can be hidden as an implementation detail.

However I am not sure putting less code in PartialVec and more code into map_inplace would simplify the code. In particular I think it's easier to see the closed PartialVec struct which provides a safe interface instead of doing the unsafe stuff in one function interleaved with the safe stuff.

aturon · 2014-09-08T17:30:41Z

@tbu- There is some amount of extra assertion checking, API documentation, and extra generality in the PartialVec methods that could go away if all the logic was made part of the map_inplace loop. Personally, I think it'd make this code simpler and more clear.

As far as interleaving safe and unsafe code, I think most of map_inplace would be unsafe, and I think that seeing both the loop and the memory operations it performs in one place will make it easier to validate the safety (because everything would fit on a single page, basically).

That said, I wouldn't block the PR on the basis of these preferences; my main concern is to hide PartialVec, which will give us the freedom to simplify things later.

I would vote for map_in_place though, mostly because the convention is to use an _ between each distinct word.

tbu- · 2014-09-11T00:06:39Z

@aturon I renamed it to map_in_place and made everything else private.

When you finish reviewing it, please tell me so I can rebase it to master.

aturon · 2014-09-12T16:21:20Z

src/libcollections/vec.rs

+    }
+}
+
+impl<T,U> Iterator<T> for PartialVec<T,U> {


Either the map_in_place method should use this iterator, or it should be dropped (as dead code).

aturon · 2014-09-12T16:23:51Z

@tbu- Looks good to me.

One minor point: now that the PartialVec interface is private, the asserts within push and into_vec could be made into debug_asserts, and the functions themselves could be marked unsafe, with map_in_place guaranteeing overall safety.

This would expand the use of unsafe a bit, but it would avoid unnecessary bounds checking on every element.

In any case, I'm happy to r+ as-is, after a rebase; we can always tweak things later. Thanks for doing this work!

tbu- · 2014-09-12T18:21:56Z

@aturon I rebased it. Will continue to improve it based on your "optional" comments once this one lands.

aturon · 2014-09-12T18:28:08Z

@tbu- Okay, headed to bors. Thanks for your persistence on this one!

tbu- · 2014-09-12T23:40:14Z

@aturon Thanks for reviewing it. Can you issue a retry? It's just a } that got lost during the rebase.

This is implemented using a new struct `PartialVec` which implements the proper drop semantics in case the conversion is interrupted by an unwind.

This specifically includes: - Fix of the tests - Remove `transmute` between `Vec`s of different types

This is important because the underlying allocator of the `Vec` passes that information to the deallocator which needs the guarantee that it is the same parameters that were also passed to the allocation function.

This replaces the now obsolete syntax `&[]` with `[].as_slice()`.

tbu- · 2014-09-14T19:37:58Z

I'm sorry for the failed build, I didn't update the tests when I rebased it to the master. Now I fixed the tests, ran make check successfully and rebased it again against current master.

This is implemented using a new struct PartialVec which implements the proper drop semantics in case the conversion is interrupted by an unwind. For the old pull requests, see #15302, #16369.

huonw reviewed Sep 5, 2014
View reviewed changes

tomjakubowski mentioned this pull request Sep 8, 2014

Replace @T in the AST with a smart pointer. #13316

Merged

aturon reviewed Sep 8, 2014
View reviewed changes

aturon reviewed Sep 12, 2014
View reviewed changes

tbu- force-pushed the pr_mapinplace branch from 6887c19 to 99dc995 Compare September 12, 2014 18:19

tbu- added 8 commits September 14, 2014 21:35

Add support for in-place map for Vecs of types with same size

dbc3cb3

This is implemented using a new struct `PartialVec` which implements the proper drop semantics in case the conversion is interrupted by an unwind.

PartialVec: Remove TODOs and rename unwrap to into_vec

af29337

Fix some of the issues mentioned in the PR on Github

23f2c78

This specifically includes: - Fix of the tests - Remove `transmute` between `Vec`s of different types

Check that the min_align_of the both types in a PartialVec matches

5efa232

This is important because the underlying allocator of the `Vec` passes that information to the deallocator which needs the guarantee that it is the same parameters that were also passed to the allocation function.

Minimize the public interface and rename it to map_in_place

b7e0969

Remove the unused Iterator implementation of the private PartialVec

79427f0

Added missing } from map_in_place rebase

7ccab3a

Fixed map_in_place tests after rustc upgrade

2c7f6ee

This replaces the now obsolete syntax `&[]` with `[].as_slice()`.

tbu- force-pushed the pr_mapinplace branch from 4b7cb27 to 2c7f6ee Compare September 14, 2014 19:36

bors closed this Sep 15, 2014

bors merged commit 2c7f6ee into rust-lang:master Sep 15, 2014

tbu- mentioned this pull request Jul 25, 2023

Comments: Learn Unsafe Rust from My Mistakes geo-ant/blog#49

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for in-place map for `Vec`s of types with same size (take 3) #16853

Add support for in-place map for `Vec`s of types with same size (take 3) #16853

tbu- commented Aug 29, 2014

huonw Sep 5, 2014

tbu- Sep 5, 2014

huonw Sep 5, 2014

thestinger Sep 5, 2014

huonw commented Sep 5, 2014

thestinger commented Sep 5, 2014

tbu- commented Sep 5, 2014

aturon Sep 8, 2014

tbu- Sep 8, 2014

aturon commented Sep 8, 2014

aturon commented Sep 8, 2014

aturon Sep 8, 2014

tbu- Sep 8, 2014

tbu- commented Sep 8, 2014

aturon commented Sep 8, 2014

tbu- commented Sep 11, 2014

aturon Sep 12, 2014

aturon commented Sep 12, 2014

tbu- commented Sep 12, 2014

aturon commented Sep 12, 2014

tbu- commented Sep 12, 2014

tbu- commented Sep 14, 2014


		let start = vec.as_mut_ptr();

		// This `as int` cast is safe, because the size of the elements of the

Add support for in-place map for Vecs of types with same size (take 3) #16853

Add support for in-place map for Vecs of types with same size (take 3) #16853

Conversation

tbu- commented Aug 29, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huonw commented Sep 5, 2014

thestinger commented Sep 5, 2014

tbu- commented Sep 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aturon commented Sep 8, 2014

aturon commented Sep 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbu- commented Sep 8, 2014

aturon commented Sep 8, 2014

tbu- commented Sep 11, 2014

Choose a reason for hiding this comment

aturon commented Sep 12, 2014

tbu- commented Sep 12, 2014

aturon commented Sep 12, 2014

tbu- commented Sep 12, 2014

tbu- commented Sep 14, 2014

Add support for in-place map for `Vec`s of types with same size (take 3) #16853

Add support for in-place map for `Vec`s of types with same size (take 3) #16853