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

Implement PartialEq/Eq for SliceInfo #689

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

Conversation

aldanor
Copy link

@aldanor aldanor commented Aug 19, 2019

Noticed that SliceInfo doesn't implement equality ops, has it been overlooked?

Not sure about the D parameter, if the indices are the same, do we consider slice info to be equal, regardless of dimension types?

Copy link
Member

@bluss bluss 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. If the indices are equal, then D1::NDIM and D2::NDIM should be too, and don't need to be checked

@@ -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

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).

@bluss
Copy link
Member

bluss commented Sep 12, 2019

@aldanor I think you'd use this code in your project? can you tell us what choice for the Dyn question makes sense for the use case you've seen?

@xd009642 xd009642 mentioned this pull request Oct 8, 2020
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.

None yet

3 participants