Adjust Vec to build on stable Rust #223

blkerby · 2020-06-19T18:49:52Z

Hi, first time contributing to the Nomicon here. I'm aware this is a somewhat large proposed change (larger than I anticipated it being when I began) but I hope it will be useful.

The chapter on implementing Vec was written to build on nightly Rust. From the discussion in 9.1 (Layout), the primary motivation for this was to get an optimized non-nullable pointer type. Since std::ptr::NonNull is now stable, this can achieved on stable Rust. This pull request makes the changes throughout the chapter necessary to implement Vec using std::ptr::NonNull instead of the unstable Unique.

Currently much of the code in the main body of the chapter does not compile on the latest nightly Rust because it relies on unstable APIs which have changed (for instance, std::alloc::oom, alloc::heap::{allocate, reallocate, deallocate} are gone, and Unique no longer implements Deref). The final code in the last section of the chapter compiles, because it has been updated to use current unstable APIs such as std::alloc::Global. This pull request updates the main body of the chapter as well as the final code so that they are consistent with each other, both now using the stable allocation functions in std::alloc (while mentioning the new unstable type std::alloc::Global in the text).

Instead of incorporating NonNull directly into Vec, RawVec, etc. as this pull request does, one alternative would be to implement a stable version of Unique using NonNull internally, and then include this Unique in Vec, RawVec, etc. I briefly started going down that pathway before concluding that for this to be convenient quite a few methods would need to be implemented for this new Unique, which would seem to create a distraction from the main goal and add an unnecessary layer of abstraction. But this would be another option if there's a preference for going that way.

Fixes #5, fixes #70, fixes #83, fixes #160

blkerby · 2020-06-19T19:03:40Z

FYI, after finishing the changes, I made a pass through the chapter and tested the cumulative code at each step, confirming that it compiles successfully on stable Rust.

skade · 2021-03-08T09:22:17Z

I want to register interest in getting this PR rebased and merged.

blkerby · 2021-04-03T11:07:55Z

I wouldn't mind rebasing and making the needed changes to get this ready again; could we get a reviewer/assignee first so we can know if the maintainers are interested in getting it merged? @JohnTitor, would you be interested in reviewing, or helping us find someone?

JohnTitor · 2021-04-03T18:03:49Z

Hey sorry for the delay! I'll try to find some time to review in a few days and it'd be great if you could rebase meanwhile.

blkerby · 2021-04-06T01:26:58Z

@JohnTitor I resolved the conflicts and rebased, so it should be ready for review whenever you're able!

JohnTitor

Sorry for the delay! Just read and the implementation itself looks good to me, just left suggestions for the style issues.

src/vec.md

src/vec-layout.md

src/vec-push-pop.md

src/vec-alloc.md

src/vec-final.md

Co-authored-by: Yuki Okushi <jtitor@2k36.org>

blkerby · 2021-04-10T03:28:04Z

Thanks, I applied all the suggested changes.

JohnTitor

LGTM! But I'm going to ask the libs team to review since this is a major change and I'm not sure if I've got the whole picture correctly (related Zulip topic: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/review.20request.20for.20the.20nomicon.20PR), I'll wait for a week or so to get some more eyes. Sorry to keep you waiting.

Amanieu · 2021-04-16T21:31:57Z

src/vec-dealloc.md

@@ -7,20 +7,17 @@ ask Rust if `T` `needs_drop` and omit the calls to `pop`. However in practice
 LLVM is *really* good at removing simple side-effect free code like this, so I
 wouldn't bother unless you notice it's not being stripped (in this case it is).


Note that the above is only true if LLVM can prove that the loop is finite. This is true in the case of Vec, but for example LLVM has no way to know that a linked list has no cycles.

Makes sense. And based on the docs for needs_drop it sounds like even in the Vec case LLVM might not eliminate the loop in debug builds. In any case, it is probably fragile to rely on LLVM for this behavior. I'm thinking of a few options for how we could address this:

Leave the code as is, but change the text to make the risk more clear.

Use a mem::needs_drop to skip the loop if T is !Drop.

Instead of an explicit loop, use ptr::drop_in_place (which is what the standard library Vec does).

Start with the current code, but within the section discuss its pitfalls and iterate on it, progressing to 2 and/or 3.

I would lean toward option 4, since currently this section is very short and could benefit from more discussion and examples to illustrate the usage of mem::needs_drop and ptr::drop_in_place. If we go with 2, 3, or 4, it will also require changes to vec-raw.md and vec-final.md for consistency. If you don't mind, do you think I could create a separate PR for this change, since it addresses a separate issue from the current PR?

I agree that option 4 sounds like the best way to go. It's fine to do this in a separate PR.

Amanieu · 2021-04-16T21:35:42Z

src/vec-final.md

-        if self.len == self.cap() { self.buf.grow(); }
+        if self.len == self.cap() {
+            self.buf.grow();
+        }

        unsafe {
            ptr::write(self.ptr().offset(self.len as isize), elem);


All the calls to .offset can be replaced with .add which avoids the need to cast to isize.

Nice! There are a couple dozen or so places throughout the chapter where this change could be made. Would you mind if I create a separate PR for this?

src/vec-insert-remove.md

jyn514 · 2021-04-16T21:56:44Z

src/vec-alloc.md

+Next we need to figure out what to actually do when we *do* want space. For that, 
+we use the global allocation functions [`alloc`][alloc], [`realloc`][realloc], 
+and [`dealloc`][dealloc] which are available in stable Rust in 
+[`std::alloc`][std_alloc]. These functions are expected to become deprecated in 
+favor of the methods of [`std::alloc::Global`][Global] after this type is stabilized. 


At some point I really want to implement rust-lang/rust#77974.

JohnTitor · 2021-05-01T08:43:07Z

It's been two weeks, @blkerby could you address Amanieu's reviews? Once it's done, I'm going to merge this finally.

blkerby · 2021-05-02T03:50:09Z

@JohnTitor I think the reviews are identifying additional ways to improve the Vec chapter that are orthogonal to the changes in this PR. If you think it makes sense, could we merge this one and then tackle those in separate PRs?

JohnTitor · 2021-05-02T08:41:02Z

I think the reviews are identifying additional ways to improve the Vec chapter that are orthogonal to the changes in this PR. If you think it makes sense, could we merge this one and then tackle those in separate PRs?

Works for me, I'd also open another PR. Thanks for your awesome work, @blkerby!

Gankra · 2021-05-02T14:10:23Z

hey, thanks a lot!!

Update books ## nomicon 4 commits in 8551afbb2ca6f5ea37fe58380318b209785e4e02..55de6fa3c1f331774da19472c9ee57d2ae9eb039 2021-04-01 21:58:50 +0900 to 2021-05-12 00:31:01 +0900 - Clarify some of the language around marking traits safe/unsafe. (rust-lang/nomicon#268) - Use pointer 'add' instead of 'offset' (rust-lang/nomicon#265) - Adjust Vec to build on stable Rust (rust-lang/nomicon#223) - Update link to c++ atomic ordering docs (rust-lang/nomicon#264) ## reference 3 commits in d23f9da8469617e6c81121d9fd123443df70595d..5aa457bf1b54bd2cd5d4cf49797f29299bdf89a7 2021-04-28 11:16:44 -0700 to 2021-05-05 08:39:22 -0700 - Explicitly state result of compound assignment (rust-lang/reference#1013) - Adjust the definition of `target_family` (rust-lang/reference#1006) - Fix typo in `Traits` (rust-lang/reference#1012) ## book 2 commits in 50dd06cb71beb27fdc0eebade5509cdcc1f821ed..55a26488ddefc8433e73a2e8352d70f7a5c7fc2b 2021-04-23 13:21:54 -0500 to 2021-05-09 12:03:18 -0500 - Past-tensify "lead" -> "led" (rust-lang/book#2717) - Merge pull request rust-lang/book#2718 from rust-lang/update-rustc ## rust-by-example 2 commits in e0a721f5202e6d9bec0aff99f10e44480c0da9e7..5f8c6da200ada77760a2fe1096938ef58151c9a6 2021-04-27 09:32:15 -0300 to 2021-04-29 08:08:01 -0300 - Fix Typo in LRBE section; closes rust-lang/rust-by-example#1434 (rust-lang/rust-by-example#1437) - Add some tests to cargo/test.md. Partially addresses rust-lang/rust-by-example#1304 (rust-lang/rust-by-example#1438) ## rustc-dev-guide 3 commits in e72b43a64925ce053dc7830e21c1a57ba00499bd..1e6c7fbda4c45e85adf63ff3f82fa9c870b1447f 2021-04-27 12:35:37 -0700 to 2021-05-10 13:38:24 +0900 - Unified CPU Requirements (rust-lang/rustc-dev-guide#1126) - add 'waiting-for-review' incantation to main contrib page (rust-lang/rustc-dev-guide#1124) - Link to Zulip search for finding the most recent check-in (rust-lang/rustc-dev-guide#1118)

JohnTitor mentioned this pull request Nov 5, 2020

Is the Nomicon still a "draft document"? #243

Closed

blkerby force-pushed the stable_vec branch from ac49a3d to 723c57b Compare April 6, 2021 01:08

Adjust Vec to build on stable Rust

83e4153

blkerby force-pushed the stable_vec branch from 723c57b to 83e4153 Compare April 6, 2021 01:14

JohnTitor added the waiting-on-review label Apr 6, 2021

JohnTitor reviewed Apr 9, 2021

View reviewed changes

Apply suggested style fixes

0e33cf6

Co-authored-by: Yuki Okushi <jtitor@2k36.org>

blkerby requested a review from JohnTitor April 16, 2021 13:31

JohnTitor approved these changes Apr 16, 2021

View reviewed changes

Amanieu reviewed Apr 16, 2021

View reviewed changes

jyn514 reviewed Apr 16, 2021

View reviewed changes

fix formatting

d918366

JohnTitor merged commit 951371f into rust-lang:master May 2, 2021

blkerby mentioned this pull request May 2, 2021

Use pointer 'add' instead of 'offset' #265

Merged

blkerby deleted the stable_vec branch May 2, 2021 19:57

ehuss mentioned this pull request May 12, 2021

Update books rust-lang/rust#85205

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust Vec to build on stable Rust #223

Adjust Vec to build on stable Rust #223

blkerby commented Jun 19, 2020 •

edited by JohnTitor

Loading

blkerby commented Jun 19, 2020

skade commented Mar 8, 2021

blkerby commented Apr 3, 2021

JohnTitor commented Apr 3, 2021

blkerby commented Apr 6, 2021 •

edited

Loading

JohnTitor left a comment

blkerby commented Apr 10, 2021

JohnTitor left a comment

Amanieu Apr 16, 2021

blkerby May 1, 2021

Amanieu May 2, 2021

Amanieu Apr 16, 2021

blkerby May 1, 2021

jyn514 Apr 16, 2021

JohnTitor commented May 1, 2021

blkerby commented May 2, 2021

JohnTitor commented May 2, 2021

Gankra commented May 2, 2021

		@@ -7,20 +7,17 @@ ask Rust if `T` `needs_drop` and omit the calls to `pop`. However in practice
		LLVM is really good at removing simple side-effect free code like this, so I
		wouldn't bother unless you notice it's not being stripped (in this case it is).

Adjust Vec to build on stable Rust #223

Adjust Vec to build on stable Rust #223

Conversation

blkerby commented Jun 19, 2020 • edited by JohnTitor Loading

blkerby commented Jun 19, 2020

skade commented Mar 8, 2021

blkerby commented Apr 3, 2021

JohnTitor commented Apr 3, 2021

blkerby commented Apr 6, 2021 • edited Loading

JohnTitor left a comment

Choose a reason for hiding this comment

blkerby commented Apr 10, 2021

JohnTitor left a comment

Choose a reason for hiding this comment

Amanieu Apr 16, 2021

Choose a reason for hiding this comment

blkerby May 1, 2021

Choose a reason for hiding this comment

Amanieu May 2, 2021

Choose a reason for hiding this comment

Amanieu Apr 16, 2021

Choose a reason for hiding this comment

blkerby May 1, 2021

Choose a reason for hiding this comment

jyn514 Apr 16, 2021

Choose a reason for hiding this comment

JohnTitor commented May 1, 2021

blkerby commented May 2, 2021

JohnTitor commented May 2, 2021

Gankra commented May 2, 2021

blkerby commented Jun 19, 2020 •

edited by JohnTitor

Loading

blkerby commented Apr 6, 2021 •

edited

Loading