Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement remove for RingBuf #19871

Merged
merged 1 commit into from
Dec 17, 2014
Merged

Implement remove for RingBuf #19871

merged 1 commit into from
Dec 17, 2014

Conversation

pczarn
Copy link
Contributor

@pczarn pczarn commented Dec 15, 2014

Includes a fix for a small mistake in fn insert which is caught by test_insert for len=15, but not len=7.

Part of #18424

r? @gankro @csherratt @huonw

pczarn
pczarn reviewed Dec 15, 2014
View reviewed changes
src/libcollections/ring_buf.rs
@@ -822,7 +822,7 @@ impl<T> RingBuf<T> {
let old_tail = self.tail;
self.tail = self.tail - 1;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I refactor these parts of insert in line with remove? IMO it's more readable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what exactly you're picturing. Feel free to do anything you believe improves code quality :)

@Gankra
Copy link
Contributor

Gankra commented Dec 15, 2014

Done review. Once my issues are addressed, I'd still appreciate a second pair of eyes as this is a bunch of unsafe code.

Actually upon reflection I think copy needs to be marked unsafe because even if you're in debug mode and the OOB is prevented, you can still use it to move uninitialized/garbage memory into a valid index or prevent a destructor from running.

@pczarn pczarn force-pushed the ring_buf-remove branch 2 times, most recently from 6d5b6d0 to 947355d Compare December 15, 2014 19:10
@pczarn
Copy link
Contributor Author

pczarn commented Dec 15, 2014

Done update. Yes, it didn't make sense that a combination of unsafe fns buffer_read and buffer_write was an equivalent of fn copy.

Gankra
Gankra reviewed Dec 15, 2014
View reviewed changes
src/libcollections/ring_buf.rs
@@ -756,9 +754,9 @@ impl<T> RingBuf<T> {
self.tail = self.wrap_index(self.tail - 1);

self.copy(self.tail, old_tail, 1);
self.copy(old_tail, old_tail + 1, i);
self.copy(old_tail, old_tail + 1, i - 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure this should be i still?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be either way

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohhh I see right because we already moved the tail! Can you toss in a comment to this effect?

@Gankra
Copy link
Contributor

Gankra commented Dec 15, 2014

Round two done.

cgaebel
cgaebel reviewed Dec 16, 2014
View reviewed changes
src/libcollections/ring_buf.rs
@@ -719,7 +717,7 @@ impl<T> RingBuf<T> {
let contiguous = self.tail <= self.head;

match (contiguous, distance_to_tail <= distance_to_head, idx >= self.tail) {
(true, true, _) if i == 0 => {
(true, true, _) if i == 0 => unsafe {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for not increasing the indent level :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I get an unnecessary unsafe block warning here.

@cgaebel
Copy link
Contributor

cgaebel commented Dec 16, 2014

It's weird that insert panics if i is out of range, but remove just returns None. Is this discontinuity on purpose?

@cgaebel
Copy link
Contributor

cgaebel commented Dec 16, 2014

I just did a read through. Didn't find anything objectionable.

I do think we should change it to 31 instead of 15 though. I know it doesn't catch any bugs at the moment, but this has me paranoid.

@Gankra
Copy link
Contributor

Gankra commented Dec 16, 2014

@cgaebel those semantics are the ones agreed upon by reform v1, for what it's worth. Feel free to make a formal argument against them in v2!

@pczarn
Copy link
Contributor Author

pczarn commented Dec 16, 2014

It's weird that insert panics if i is out of range, but remove just returns None. Is this discontinuity on purpose?

Vec does that.

I do think we should change it to 31 instead of 15 though. I know it doesn't catch any bugs at the moment, but this has me paranoid.

A situation where insertion happened closer to tail in the head section (discontiguous), and at a non-zero index, wasn't possible with 7. IMO >15 doesn't really matter.

@pczarn pczarn force-pushed the ring_buf-remove branch 2 times, most recently from f9ba541 to bdbdd59 Compare December 16, 2014 23:30
@cgaebel
Copy link
Contributor

cgaebel commented Dec 16, 2014

Again, looks good to me. Just making sure those were on purpose and not overlooked.

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Dec 17, 2014
@alexcrichton
rollup merge of rust-lang#19871: pczarn/ring_buf-remove
272f203 
Includes a fix for a small mistake in `fn insert` which is caught by test_insert for len=15, but not len=7.

Part of rust-lang#18424

r? @gankro @csherratt @huonw
@bors bors merged commit 59d4153 into rust-lang:master Dec 17, 2014
@pczarn pczarn deleted the ring_buf-remove branch January 14, 2015 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pczarn @Gankra @cgaebel @bors