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

Implement PartialEq/Eq for SliceInfo #689

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Implement PartialEq/Eq for SliceInfo #689

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
19 changes: 19 additions & 0 deletions src/slice.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,25 @@ pub struct SliceInfo<T: ?Sized, D: Dimension> {
indices: T,
}

impl<T1, D1, T2, D2> PartialEq<SliceInfo<T2, D2>> for SliceInfo<T1, D1>
Copy link
Member

@jturner314 jturner314 Sep 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Why are D1 and D2 allowed to be different types? If D1 != D2, the SliceInfo instances are guaranteed to be unequal. I would think it would be more useful to catch that case at compile time instead of waiting until runtime.

Copy link
Author

Choose a reason for hiding this comment

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

What if one is Dyn?

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, I forgot that it's possible to create SliceInfo instances without using the s![] macro.

There's still some question in my mind, though, whether SliceInfo<T1, D1> and SliceInfo<T2, D2> should be considered equal (if their contents are equal) even if D1 and D2 are different. Both of these interpretations make sense to me:

  1. They should be equal because they will slice arrays in the same way (same indices and resulting shape).

  2. They should be unequal because the arrays resulting from slicing will have a different dimension types.

Since both interpretations make sense, I'd prefer to avoid the ambiguity by requiring D1 and D2 to be the same in the PartialEq impl. If someone wants to compare the contents of SliceInfo instances with different dimension types, they can just use info1.as_ref() == info2.as_ref().

Copy link
Member

Choose a reason for hiding this comment

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

I wonder why we encourage this constant .as_ref() calling. ("This is not how conversion traits are supposed to be used, and it's fragile" is the short story).

We should add a method to them, if it's intended that you convert them like this in open code (not as part of converting function arguments).

where
T1: AsRef<[SliceOrIndex]>,
T2: AsRef<[SliceOrIndex]>,
D1: Dimension,
D2: Dimension,
{
fn eq(&self, other: &SliceInfo<T2, D2>) -> bool {
self.indices.as_ref() == other.indices.as_ref()
}
}

impl<T, D> Eq for SliceInfo<T, D>
where
T: AsRef<[SliceOrIndex]>,
D: Dimension,
{
}

impl<T: ?Sized, D> Deref for SliceInfo<T, D>
where
D: Dimension,
Expand Down