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.

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

Add append() and split_off() to DList as per coll. reform. #20406

Merged
merged 1 commit into from
Jan 11, 2015

Conversation

TimDumol
Copy link
Contributor

@TimDumol TimDumol commented Jan 1, 2015

Implements the append() and split_off() methods proposed by the collections reform part 2 RFC.

RFC: rust-lang/rfcs#509
Tracking issue: #19986

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@TimDumol
Copy link
Contributor Author

TimDumol commented Jan 1, 2015

This is my first code PR, so I hope I haven't messed it up!

@Gankra
Copy link
Contributor

Gankra commented Jan 1, 2015

This should just replace the old append method. That has been the intent from the beginning.

@@ -263,9 +263,12 @@ impl<T> DList<T> {
});
}

/// Adds all elements from `other` to the end of the list.
/// Destructively adds all elements from `other` to the end of the list.
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 destructively is the right word here. "moves" seems more right.

@alexcrichton alexcrichton assigned Gankra and unassigned alexcrichton Jan 1, 2015
#[stable]
pub fn split_off(&mut self, at: uint) -> DList<T> {
let len = self.len();
if len <= at {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would make this an assert!(len <= at, "message")

@Gankra
Copy link
Contributor

Gankra commented Jan 1, 2015

Done first-pass review.

@TimDumol TimDumol force-pushed the dlist-append-split-off branch 3 times, most recently from f15cc5b to 60d2b72 Compare January 3, 2015 00:39
@TimDumol
Copy link
Contributor Author

TimDumol commented Jan 3, 2015

@gankro Thanks for the quick review! I fixed all of the issues you've noted.

@TimDumol TimDumol changed the title Add append_mut() and split_off() to DList as per coll. reform. Add append() and split_off() to DList as per coll. reform. Jan 3, 2015
@TimDumol TimDumol force-pushed the dlist-append-split-off branch from 60d2b72 to 678b857 Compare January 3, 2015 01:12
@Gankra
Copy link
Contributor

Gankra commented Jan 4, 2015

This LGTM, but I like to get a secondary review for most collections code (since it's tricksy).

r? @huonw

@TimDumol TimDumol force-pushed the dlist-append-split-off branch from 678b857 to 8ce6e06 Compare January 5, 2015 05:36
/// Splits the list into two at the given index. Returns everything after the given index,
/// including the index.
///
/// This operation should compute in O(n) time.
Copy link
Member

Choose a reason for hiding this comment

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

Should, or does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just followed the convention of some other doc strings in the file that mention asymptotic performance (e.g., https://github.com/rust-lang/rust/pull/20406/files#diff-4103b030c3b97fcdc52e866d1cb300fbR363). I'm indifferent to the phrasing though, so if you insist, I'll change it to "This operation computes in O(n) time."

Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw I'm happy to leave it like this, to be handled by a more comprehensive collection doc conventions pass (which I'm sort-of working on).

@emberian
Copy link
Member

emberian commented Jan 5, 2015

r=me with fixes

@TimDumol TimDumol force-pushed the dlist-append-split-off branch from 8ce6e06 to 245b0b5 Compare January 7, 2015 15:05
@TimDumol TimDumol force-pushed the dlist-append-split-off branch from 245b0b5 to d0bc031 Compare January 9, 2015 04:13
@TimDumol
Copy link
Contributor Author

TimDumol commented Jan 9, 2015

@cmr Oops, my bad! I removed the submodule update.

@Gankra
Copy link
Contributor

Gankra commented Jan 11, 2015

@cmr does this meet your requirements?

@emberian
Copy link
Member

Yes

bors added a commit that referenced this pull request Jan 11, 2015
Implements the `append()` and `split_off()` methods proposed by the collections reform part 2 RFC.

RFC: rust-lang/rfcs#509
Tracking issue: #19986
@bors bors merged commit d0bc031 into rust-lang:master Jan 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants