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

Provide IndexMut for Vector<T: Clone> (Issue #30) #37

Merged
merged 5 commits into from
Sep 13, 2018

Conversation

SilensAngelusNex
Copy link
Contributor

Here's some work towards #30. I don't think its possible to implement for Vector<T: !Clone>.

I'm pretty new to Rust, so any constructive criticism would be appreciated.

@SilensAngelusNex SilensAngelusNex changed the title Provide IndexMut for Vector<T: Clone> Provide IndexMut for Vector<T: Clone> (Issue #30) Sep 11, 2018
@orium
Copy link
Owner

orium commented Sep 11, 2018

Thanks for your contribution!

Looking at travis, you need to do a cargo +nightly fmt. You can run ./tools/check.sh locally to check that everything is ok.

Copy link
Owner

@orium orium left a comment

Choose a reason for hiding this comment

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

Looks good! Just a few minor details and it is good to go :)

@@ -444,6 +464,25 @@ impl<T> Vector<T> {
}
}

impl<T: Clone> Vector<T> {
/// Gets a mutable reference to an element. If the element is shared, this Vector will be cloned.
/// Returns **None** if and only if the given **index** is out of range.
Copy link
Owner

@orium orium Sep 12, 2018

Choose a reason for hiding this comment

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

backtick None and index instead of bold

@@ -444,6 +464,25 @@ impl<T> Vector<T> {
}
}

impl<T: Clone> Vector<T> {
/// Gets a mutable reference to an element. If the element is shared, this Vector will be cloned.
Copy link
Owner

Choose a reason for hiding this comment

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

I think it is best to say "If the element is shared it will be cloned.". It is true for all mutable operations that part of the vector will be cloned.

v2[4] = String::from("cloning");
v2[5].make_ascii_uppercase();

for i in 0..v1.len() {
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of the loop we can just

assert_eq!(v1, expected1);
assert_eq!(v2, expected2);

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 was looping through because the vectors are not actually the same type. I'll change the implementation for the PartialEq and PartialOrd so that this does work.

@orium orium merged commit fa48b23 into orium:master Sep 13, 2018
@orium
Copy link
Owner

orium commented Sep 13, 2018

Merged! Thanks!

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.

2 participants