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

Improve PartialEq for slices #26884

Merged
merged 1 commit into from
Jul 9, 2015
Merged

Improve PartialEq for slices #26884

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions src/libcore/slice.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1459,12 +1459,30 @@ pub mod bytes {
#[stable(feature = "rust1", since = "1.0.0")]
impl<A, B> PartialEq<[B]> for [A] where A: PartialEq<B> {
fn eq(&self, other: &[B]) -> bool {
self.len() == other.len() &&
order::eq(self.iter(), other.iter())
if self.len() != other.len() {
return false;
}

for i in 0..self.len() {
if !self[i].eq(&other[i]) {
return false;
}
}

true
}
fn ne(&self, other: &[B]) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this has already been merged and looks fantastic but I, as a Rust n00b, had a question as to why this isn't just

fn ne(&self, other: &[B]) -> bool {
    !self.eq(other);
}

Is it a performance issue to avoid the duplication?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or even just drop this and use the default implementation of ne() from the trait definition.

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 be interested to know this as well. Could it be because there is call to ne on line 1480, which may not be the same as !eq?

Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation for PartialEq states: "Any manual implementation of ne must respect the rule that eq is a strict inverse of ne; that is, !(a == b) if and only if a != b."
Under this assumption, ne can always re-use the implementation from the trait definition. I would expect the compiler to do a good job optimising it as it can fold the negation either on the possible outcomes or on the ne usage. Unless some benchmarking suggests otherwise, I would assume that avoiding the duplication is preferred.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose we'll have to wait to hear back from @dotdash then :-)

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 don't remember exactly why I didn't change that, but probably I was just too lazy to come up with enough useful testcases (involving at least various calling- and inlining-scenarios) to make sure that relying on the simple negation implementation doesn't affect performance negatively. Benchmarking welcome!

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll see if I can find some time this weekend to test it out.

self.len() != other.len() ||
order::ne(self.iter(), other.iter())
if self.len() != other.len() {
return true;
}

for i in 0..self.len() {
if self[i].ne(&other[i]) {
return true;
}
}

false
}
}

Expand Down