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

Remove unnecessary as[_mut]_slice/to_string() calls #19378

Closed
wants to merge 35 commits into from

Conversation

japaric
Copy link
Member

@japaric japaric commented Nov 28, 2014

Now that we have an overloaded comparison (==) operator, and that Vec/String deref to [T]/str on method calls, many as_slice()/as_mut_slice()/to_string() calls have become redundant. This patch removes them. These were the most common patterns:

  • assert_eq(test_output.as_slice(), "ground truth") -> assert_eq(test_output, "ground truth")
  • assert_eq(test_output, "ground truth".to_string()) -> assert_eq(test_output, "ground truth")
  • vec.as_mut_slice().sort() -> vec.sort()
  • vec.as_slice().slice(from, to) -> vec.slice(from_to)

Note that e.g. a_string.push_str(b_string.as_slice()) has been left untouched in this PR, since we first need to settle down whether we want to favor the &*b_string or the b_string[] notation.

This is rebased on top of #19167

cc @alexcrichton @aturon

@rust-highfive
Copy link
Collaborator

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!


impl<'a, A, B> PartialEq<Vec<B>> for &'a [A] where A: PartialEq<B> {
#[inline]
fn eq(&self, other: &Vec<B>) -> bool { PartialEq::eq(*self, &**other) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why the impls above and below compare &**self, whereas this only compares *self?

Copy link
Member Author

Choose a reason for hiding this comment

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

These three implementations merely defer their job to the impl PartialEq<[B]> for [A] one, where the signature of eq is fn eq(&Self = &[A], &[B]). The *self vs &**self is just about playing the deref game:

  • In this case self has type &&[A] so *self has type &[A]
  • In case above, self has type &Vec<A>, *self has type Vec<A>, and the next * actually triggers Deref<[A]>, so &**self has type &[A].
  • In case below, self has type &&mut [A], *self has type "&mut [A]" (yes, I know the compiler won't actually let you do that), and the next&*is a "explicit immutable reborrow" to get&[A]`.

Equivalently, I could have written &**self in all these three cases.

@japaric
Copy link
Member Author

japaric commented Nov 28, 2014

BTW, @bstrie your questions belong to the PR #19167, where the == was overloaded. This PR is only about removing some method calls that became redundant after that change.

@alexcrichton
Copy link
Member

This is awesome, thanks @japaric! r=me, just waiting on @aturon for #19167

@aturon
Copy link
Member

aturon commented Dec 1, 2014

Hey! I've been offline for Thanksgiving but just got back to #19167. Hopefully we can move this through quickly.

@aturon
Copy link
Member

aturon commented Dec 4, 2014

@alexcrichton just a heads-up, #19167 has been approved.

@japaric
Copy link
Member Author

japaric commented Dec 4, 2014

@alexcrichton rebased, and tested.

@japaric japaric force-pushed the no-as-slice branch 3 times, most recently from 4ae48bd to f569d5c Compare December 5, 2014 23:09
bors added a commit that referenced this pull request Dec 6, 2014
Now that we have an overloaded comparison (`==`) operator, and that `Vec`/`String` deref to `[T]`/`str` on method calls, many `as_slice()`/`as_mut_slice()`/`to_string()` calls have become redundant. This patch removes them. These were the most common patterns:

- `assert_eq(test_output.as_slice(), "ground truth")` -> `assert_eq(test_output, "ground truth")`
- `assert_eq(test_output, "ground truth".to_string())` -> `assert_eq(test_output, "ground truth")`
- `vec.as_mut_slice().sort()` -> `vec.sort()`
- `vec.as_slice().slice(from, to)` -> `vec.slice(from_to)`

---

Note that e.g. `a_string.push_str(b_string.as_slice())` has been left untouched in this PR, since we first need to settle down whether we want to favor the `&*b_string` or the `b_string[]` notation.

This is rebased on top of #19167

cc @alexcrichton @aturon
bors added a commit that referenced this pull request Dec 7, 2014
Now that we have an overloaded comparison (`==`) operator, and that `Vec`/`String` deref to `[T]`/`str` on method calls, many `as_slice()`/`as_mut_slice()`/`to_string()` calls have become redundant. This patch removes them. These were the most common patterns:

- `assert_eq(test_output.as_slice(), "ground truth")` -> `assert_eq(test_output, "ground truth")`
- `assert_eq(test_output, "ground truth".to_string())` -> `assert_eq(test_output, "ground truth")`
- `vec.as_mut_slice().sort()` -> `vec.sort()`
- `vec.as_slice().slice(from, to)` -> `vec.slice(from_to)`

---

Note that e.g. `a_string.push_str(b_string.as_slice())` has been left untouched in this PR, since we first need to settle down whether we want to favor the `&*b_string` or the `b_string[]` notation.

This is rebased on top of #19167

cc @alexcrichton @aturon
@japaric
Copy link
Member Author

japaric commented Dec 7, 2014

Ahh, sorry, a recent PR modified an existing test, and I missed that change during my last rebase. I've now amended my mistake and make checked the whole thing. @alexcrichton re-r?

@japaric
Copy link
Member Author

japaric commented Dec 7, 2014

😢 A syntax error on the android tests, which didn't get parsed when I ran make check. I grepped each commit diff for the same sort of error, but didn't find any other occurrence. r? @alexcrichton

bors added a commit that referenced this pull request Dec 8, 2014
Now that we have an overloaded comparison (`==`) operator, and that `Vec`/`String` deref to `[T]`/`str` on method calls, many `as_slice()`/`as_mut_slice()`/`to_string()` calls have become redundant. This patch removes them. These were the most common patterns:

- `assert_eq(test_output.as_slice(), "ground truth")` -> `assert_eq(test_output, "ground truth")`
- `assert_eq(test_output, "ground truth".to_string())` -> `assert_eq(test_output, "ground truth")`
- `vec.as_mut_slice().sort()` -> `vec.sort()`
- `vec.as_slice().slice(from, to)` -> `vec.slice(from_to)`

---

Note that e.g. `a_string.push_str(b_string.as_slice())` has been left untouched in this PR, since we first need to settle down whether we want to favor the `&*b_string` or the `b_string[]` notation.

This is rebased on top of #19167

cc @alexcrichton @aturon
@bors bors closed this Dec 8, 2014
@japaric japaric deleted the no-as-slice branch December 16, 2014 02:10
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