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

Tweaks to equality comparisons for slices/arrays/vectors #22320

Merged
merged 1 commit into from
Feb 23, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
83 changes: 24 additions & 59 deletions src/libcollections/vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1501,69 +1501,34 @@ impl<T> Extend<T> for Vec<T> {
}
}

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

macro_rules! impl_eq {
($lhs:ty, $rhs:ty) => {
impl<'b, A, B> PartialEq<$rhs> for $lhs where A: PartialEq<B> {
#[inline]
fn eq(&self, other: &$rhs) -> bool { PartialEq::eq(&**self, &**other) }
#[inline]
fn ne(&self, other: &$rhs) -> bool { PartialEq::ne(&**self, &**other) }
}

impl<'b, A, B> PartialEq<$lhs> for $rhs where B: PartialEq<A> {
#[inline]
fn eq(&self, other: &$lhs) -> bool { PartialEq::eq(&**self, &**other) }
#[inline]
fn ne(&self, other: &$lhs) -> bool { PartialEq::ne(&**self, &**other) }
}
__impl_slice_eq1! { Vec<A>, Vec<B> }
__impl_slice_eq2! { Vec<A>, &'b [B] }
__impl_slice_eq2! { Vec<A>, &'b mut [B] }
__impl_slice_eq2! { CowVec<'a, A>, &'b [B], Clone }
__impl_slice_eq2! { CowVec<'a, A>, &'b mut [B], Clone }
__impl_slice_eq2! { CowVec<'a, A>, Vec<B>, Clone }

macro_rules! array_impls {
($($N: expr)+) => {
$(
// NOTE: some less important impls are omitted to reduce code bloat
__impl_slice_eq2! { Vec<A>, [B; $N] }
__impl_slice_eq2! { Vec<A>, &'b [B; $N] }
// __impl_slice_eq2! { Vec<A>, &'b mut [B; $N] }
// __impl_slice_eq2! { CowVec<'a, A>, [B; $N], Clone }
// __impl_slice_eq2! { CowVec<'a, A>, &'b [B; $N], Clone }
// __impl_slice_eq2! { CowVec<'a, A>, &'b mut [B; $N], Clone }
)+
}
}

impl_eq! { Vec<A>, &'b [B] }
impl_eq! { Vec<A>, &'b mut [B] }

impl<'a, A, B> PartialEq<Vec<B>> for Cow<'a, [A]> where A: PartialEq<B> + Clone {
#[inline]
fn eq(&self, other: &Vec<B>) -> bool { PartialEq::eq(&**self, &**other) }
#[inline]
fn ne(&self, other: &Vec<B>) -> bool { PartialEq::ne(&**self, &**other) }
array_impls! {
0 1 2 3 4 5 6 7 8 9
10 11 12 13 14 15 16 17 18 19
20 21 22 23 24 25 26 27 28 29
30 31 32
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little worried about the impact this has on the metadata, this seems like a huge number of implementations being thrown in, not all of which are really that necessary. Can you make sure we're not blowing up the metadata by a few megabytes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just by comparing the sizes of produced rlibs before and after or is there a smarter way to compare rlibs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexcrichton
Sadly, the bloat is significant.

libcore: 36MB -> 39MB
libcollections: 11MB -> 18MB (15MB without CowVec impls)
Besides, if I purge libcore/array.rs completely, then libcore.rlib loses another 6MB and becomes 30MB.

That's one more reason, why we need something like this sooner.

I think the reasonable subsequent actions would be:

  1. Keep the macros
  2. Keep the "status quo" impls
  3. Add the impls crucial for Byte string literals should have a type of a fixed size #18465
    Then the bloat should be tolerable

Copy link
Member

Choose a reason for hiding this comment

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

Hm yeah this is quite unfortunate how big a blowup this is (11MB => 18MB is huge!). I suspect there is a huge trove of optimizations that we could exploit for metadata layout, but unfortunately it will take time to examine.

I think that your proposed sequence of actions should be fine.

Copy link
Member

Choose a reason for hiding this comment

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

(thanks for analyzing by the way!)

Also, for the future, I analyze metadata by running ar x libfoo.rlib and then looking at rust.metadata.bin to see how big it is.

}

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

macro_rules! impl_eq_for_cowvec {
($rhs:ty) => {
impl<'a, 'b, A, B> PartialEq<$rhs> for Cow<'a, [A]> where A: PartialEq<B> + Clone {
#[inline]
fn eq(&self, other: &$rhs) -> bool { PartialEq::eq(&**self, &**other) }
#[inline]
fn ne(&self, other: &$rhs) -> bool { PartialEq::ne(&**self, &**other) }
}

impl<'a, 'b, A, B> PartialEq<Cow<'a, [A]>> for $rhs where A: Clone, B: PartialEq<A> {
#[inline]
fn eq(&self, other: &Cow<'a, [A]>) -> bool { PartialEq::eq(&**self, &**other) }
#[inline]
fn ne(&self, other: &Cow<'a, [A]>) -> bool { PartialEq::ne(&**self, &**other) }
}
}
}

impl_eq_for_cowvec! { &'b [B] }
impl_eq_for_cowvec! { &'b mut [B] }

#[stable(feature = "rust1", since = "1.0.0")]
impl<T: PartialOrd> PartialOrd for Vec<T> {
#[inline]
Expand Down Expand Up @@ -2480,7 +2445,7 @@ mod tests {
fn test_into_boxed_slice() {
let xs = vec![1, 2, 3];
let ys = xs.into_boxed_slice();
assert_eq!(ys, [1, 2, 3]);
assert_eq!(&*ys, [1, 2, 3]);
}

#[test]
Expand Down
51 changes: 8 additions & 43 deletions src/libcore/array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@ use cmp::{PartialEq, Eq, PartialOrd, Ord, Ordering};
use fmt;
use hash::{Hash, self};
use iter::IntoIterator;
use marker::Copy;
use ops::Deref;
use marker::{Copy, Sized};
use option::Option;
use slice::{Iter, IterMut, SliceExt};

Expand Down Expand Up @@ -76,47 +75,13 @@ macro_rules! array_impls {
}
}

#[stable(feature = "rust1", since = "1.0.0")]
impl<A, B> PartialEq<[B; $N]> for [A; $N] where A: PartialEq<B> {
#[inline]
fn eq(&self, other: &[B; $N]) -> bool {
&self[..] == &other[..]
}
#[inline]
fn ne(&self, other: &[B; $N]) -> bool {
&self[..] != &other[..]
}
}

#[stable(feature = "rust1", since = "1.0.0")]
impl<'a, A, B, Rhs> PartialEq<Rhs> for [A; $N] where
A: PartialEq<B>,
Rhs: Deref<Target=[B]>,
{
#[inline(always)]
fn eq(&self, other: &Rhs) -> bool {
PartialEq::eq(&self[..], &**other)
}
#[inline(always)]
fn ne(&self, other: &Rhs) -> bool {
PartialEq::ne(&self[..], &**other)
}
}

#[stable(feature = "rust1", since = "1.0.0")]
impl<'a, A, B, Lhs> PartialEq<[B; $N]> for Lhs where
A: PartialEq<B>,
Lhs: Deref<Target=[A]>
{
#[inline(always)]
fn eq(&self, other: &[B; $N]) -> bool {
PartialEq::eq(&**self, &other[..])
}
#[inline(always)]
fn ne(&self, other: &[B; $N]) -> bool {
PartialEq::ne(&**self, &other[..])
}
}
// NOTE: some less important impls are omitted to reduce code bloat
__impl_slice_eq1! { [A; $N], [B; $N] }
__impl_slice_eq2! { [A; $N], [B] }
__impl_slice_eq2! { [A; $N], &'b [B] }
__impl_slice_eq2! { [A; $N], &'b mut [B] }
// __impl_slice_eq2! { [A; $N], &'b [B; $N] }
// __impl_slice_eq2! { [A; $N], &'b mut [B; $N] }

#[stable(feature = "rust1", since = "1.0.0")]
impl<T:Eq> Eq for [T; $N] { }
Expand Down
50 changes: 50 additions & 0 deletions src/libcore/cmp_macros.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// Utility macros for implementing PartialEq on slice-like types

#![doc(hidden)]

#[macro_export]
Copy link
Member

Choose a reason for hiding this comment

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

I think that this macro wants to be marked with #[macro_use] instead of having these be #[macro_export] (which adds these macros to the prelude of the standard library).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The macros are defined in libcore and used in libcollection. Isn't macro_export needed for cross-crate macro use?

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, good point! Carry on then!

macro_rules! __impl_slice_eq1 {
($Lhs: ty, $Rhs: ty) => {
#[stable(feature = "rust1", since = "1.0.0")]
impl<'a, 'b, A, B> PartialEq<$Rhs> for $Lhs where A: PartialEq<B> {
#[inline]
fn eq(&self, other: &$Rhs) -> bool { &self[..] == &other[..] }
#[inline]
fn ne(&self, other: &$Rhs) -> bool { &self[..] != &other[..] }
}
}
}

#[macro_export]
macro_rules! __impl_slice_eq2 {
($Lhs: ty, $Rhs: ty) => {
__impl_slice_eq2! { $Lhs, $Rhs, Sized }
};
($Lhs: ty, $Rhs: ty, $Bound: ident) => {
#[stable(feature = "rust1", since = "1.0.0")]
impl<'a, 'b, A: $Bound, B> PartialEq<$Rhs> for $Lhs where A: PartialEq<B> {
#[inline]
fn eq(&self, other: &$Rhs) -> bool { &self[..] == &other[..] }
#[inline]
fn ne(&self, other: &$Rhs) -> bool { &self[..] != &other[..] }
}

#[stable(feature = "rust1", since = "1.0.0")]
impl<'a, 'b, A: $Bound, B> PartialEq<$Lhs> for $Rhs where B: PartialEq<A> {
#[inline]
fn eq(&self, other: &$Lhs) -> bool { &self[..] == &other[..] }
#[inline]
fn ne(&self, other: &$Lhs) -> bool { &self[..] != &other[..] }
}
}
}
3 changes: 3 additions & 0 deletions src/libcore/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@
#[macro_use]
mod macros;

#[macro_use]
mod cmp_macros;

#[path = "num/float_macros.rs"]
#[macro_use]
mod float_macros;
Expand Down
2 changes: 1 addition & 1 deletion src/libstd/ffi/c_str.rs
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ impl CStr {

impl PartialEq for CStr {
fn eq(&self, other: &CStr) -> bool {
self.to_bytes().eq(&other.to_bytes())
self.to_bytes().eq(other.to_bytes())
}
}
impl Eq for CStr {}
Expand Down