LinkedList: Incremental improvements #26050
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
r? @gankro Probably best reviewed commit by commit. |
bluss
force-pushed
the
linked-list
branch
3 times, most recently
from
f7bce33 to
55ad5f2
Compare
| @@ -609,12 +635,21 @@ impl<T> LinkedList<T> { | |||
| length: len - at | |||
| }; | |||
|
|
|||
| // Swap split_node.next with list_head (which is None), nulling out split_node.next, | |||
| // as it is the new tail. | |||
| mem::swap(&mut split_node.resolve().unwrap().next, &mut splitted_list.list_head); | |||
There was a problem hiding this comment.
Actually, this could have just been done in the ctor of splitted_list all along, right?
let mut splitted_list = LinkedList {
list_head: split_node.resolve().unwrap().next.take(),
list_tail: self.list_tail,
length: len - at,
}
|
I've updated split_off, it's sort of a compromise now (but I like it). Use .unwrap() in one place, but no map where it's just for side effects. |
|
I'll squash that in later. |
|
I couldn't help renaming splitted, I refuse this neologism. |
| second_part_head = split_node.resolve_mut().unwrap().next.take(); | ||
| match second_part_head { | ||
| None => {} | ||
| Some(ref mut head) => head.prev = Rawlink::none(), |
There was a problem hiding this comment.
I just know someone's going to submit a PR for "replace match with if-let/map/unwrap", but I'm going to accept this ;)
|
r=me once you've done whatever squashing you feel is appropriate. |
|
@bors: r=Gankro |
|
📌 Commit 32037a5 has been approved by |
|
wheee! |
The recent bug that was found in LinkedList reminded me of some general cleanup that's been waiting for some time. - Use a loop from the front in Drop, it works just as well and without an unsafe block - Change Rawlink methods to use `unsafe` in an idiomatic way. This does mean that we need an unsafe block for each dereference of a raw link. Even then, the extent of unsafe-critical code is even larger of course, since safety depends on the whole data structure's integrity. This is a general problem we are aware of. - Some cleanup just to try to decrease the amount of Rawlink handling.
The recent bug that was found in LinkedList reminded me of some general cleanup
that's been waiting for some time.
unsafein an idiomatic way. This does mean thatwe need an unsafe block for each dereference of a raw link. Even then, the extent
of unsafe-critical code is even larger of course, since safety depends on the whole
data structure's integrity. This is a general problem we are aware of.