• refactor(allocator/vec2): rename parameters and method name to align with std #9858 : 2 dependent PRs (#9855 , #9857 )
• feat(allocator/vec2): align the retain method with the standard implementation #9752
• feat(allocator/vec): remove ManuallyDrop wrapper #9742
• feat(allocator): remove drop operations from Vec2 #9679
• feat(allocator): replace allocator_ap2's Vec with Vec2 #9656
• feat(allocator): add Vec2::retain_mut method #9655
• feat(allocator): connect Vec2 module and make it compile #9647 👈 (View in Graphite)
• refactor(allocator/vec): disable lint warnings in vec2 files #9730
• docs(allocator/vec): fix link in doc comment for Vec2 #9729
• refactor(allocator/vec): comment out feature-gated methods #9728
• main

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• 0-merge - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

This stack of pull requests is managed by Graphite. Learn more about stacking.

CodSpeed Performance Report

Merging #9647 will not alter performance

_{Comparing 03-10-feat_allocator_add_vec2_module_and_make_it_compile (3943563) with 03-12-refactor_allocator_vec_disable_lint_warnings_in_vec2_files (5c5e010)}

Summary

✅ 6 untouched benchmarks
🆕 27 new benchmarks

Benchmarks breakdown

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

@Dunqing I hope you don't mind, but I've split this PR up into 4 parts (the preceding 3 PRs are pulled out of this one), just so the diffs are easier to read.

Annoyingly Graphite erases all record of multiple commits when it merges a PR, so if you want to keep a "paper trail" without the diffs of different commits combined, they have to be in separate PRs.

The only substantial change I made was to disable the clippy lints which bumpalo's code triggers (#9730), rather than fixing them all, as you had. Some of the changes related to pointers and arithmetic overflow are a bit subtle, and I don't 100% trust clippy's suggestions, so I thought it'd be better to defer making those changes.

I suggest that we get Vec2 connected up, solve the lifetime problems, and check it all works before making changes to bumpalo's code. Once we've got that far, we can re-enable lint rules and fix the warnings in batches - do the simple ones first, and then tackle the pointer-related ones in a separate PR - a smaller diff in that PR will make it easier to see what we're doing and check our work. The pointer stuff being correct is critical for safety, so I personally I think it's worthwhile proceeding cautiously with that.

I've been through this PR in detail and verified that the changes to alloc, dealloc and realloc calls are equivalent to the originals. It's kind of weird that we have to use the allocator_api2::Allocator methods, but Bumpalo doesn't expose its internal Alloc trait, so I think you're right that this is the only solution.

So... if you're happy with the changes I've made, I think we can merge the stack up to and including this PR.

I'd like to take a closer look at retain_mut before we merge that one. And I think maybe I have a solution to the lifetime problems in #9656. But it's a bit of a brain-bender!

In case I've screwed anything up, all the commits from your original stack before I made any changes are backed up on overlookmotel/vec-rewrite-original branch.

@Dunqing I hope you don't mind, but I've split this PR up into 4 parts (the preceding 3 PRs are pulled out of this one), just so the diffs are easier to read.

Annoyingly Graphite erases all record of multiple commits when it merges a PR, so if you want to keep a "paper trail" without the diffs of different commits combined, they have to be in separate PRs.

Thank you for splitting this PR, yes, this is also what I am thinking if I should do these different things in separate PRs. Currently, the whole stack is more clear about what I am doing, so of course I very much like you to do this.

The only substantial change I made was to disable the clippy lints which bumpalo's code triggers (#9730), rather than fixing them all, as you had. Some of the changes related to pointers and arithmetic overflow are a bit subtle, and I don't 100% trust clippy's suggestions, so I thought it'd be better to defer making those changes.

I suggest that we get Vec2 connected up, solve the lifetime problems, and check it all works before making changes to bumpalo's code. Once we've got that far, we can re-enable lint rules and fix the warnings in batches - do the simple ones first, and then tackle the pointer-related ones in a separate PR - a smaller diff in that PR will make it easier to see what we're doing and check our work. The pointer stuff being correct is critical for safety, so I personally I think it's worthwhile proceeding cautiously with that.

Yes, I agree!

I've been through this PR in detail and verified that the changes to alloc, dealloc and realloc calls are equivalent to the originals. It's kind of weird that we have to use the allocator_api2::Allocator methods, but Bumpalo doesn't expose its internal Alloc trait, so I think you're right that this is the only solution.

Yes, this is why I forked Bumpalo and exposed the alloc module in the last experiment.

So... if you're happy with the changes I've made, I think we can merge the stack up to and including this PR.

I'd like to take a closer look at retain_mut before we merge that one. And I think maybe I have a solution to the lifetime problems in #9656. But it's a bit of a brain-bender!

In case I've screwed anything up, all the commits from your original stack before I made any changes are backed up on overlookmotel/vec-rewrite-original branch.

I am okay with all changes to this stack. The decision to click the merge button is up to you, merge it when you think the timing is right now.

Merge activity

• Mar 12, 10:43 PM EDT: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Mar 12, 10:43 PM EDT: A user added this pull request to the Graphite merge queue.
• Mar 12, 11:02 PM EDT: A user merged this pull request with the Graphite merge queue.

OK great. Thanks for reviewing my changes. Merging the bottom 4 PRs now.