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

Feature/span equality operators #670

Conversation

KurtisT
Copy link
Contributor

@KurtisT KurtisT commented Feb 9, 2023

Added equals and not equals operator for spans.

@jwellbelove jwellbelove changed the base branch from master to pull-request/#670-span-equality-operators February 9, 2023 16:50
@jwellbelove jwellbelove merged commit 36a9d70 into ETLCPP:pull-request/#670-span-equality-operators Feb 9, 2023
@jwellbelove
Copy link
Contributor

I gave the accepted changes some thought last night and I see a problem.
What does it mean to compare a span?
Should we compare the pointers to the range, or the values contained within that range (shallow or deep comparison)?
Both comparisons could be argued as being 'correct'.

This may be the reason why comparisons were not included for std::span.

I think that a deep comparison is more valid in that it reflects what a comparison of a container would do.

A better implementation may be to first compare the pointers/size, and, if they do not match, compare the contents of the spans.

@jwellbelove
Copy link
Contributor

I've updated the pulled code to do a deep comparison.

@KurtisT
Copy link
Contributor Author

KurtisT commented Feb 10, 2023

I had the same question about doing a shallow vs. deep comparison and came to the opposite conclusion. My reasoning was just that if the underlying containers are different, the spans would be operating on different containers and would not be considered the same. Even if the data in the containers happen to be the same. I agree that both comparisons can be thought of as correct so if you think a deep compare is more valid that's good with me.

@jwellbelove
Copy link
Contributor

Maybe additional explicit comparison member functions could be useful?

Compare internal pointers
bool is_equal_span_range(const etl::span<U>& other) const

Compare pointed to data
bool is_equal_span_data(const etl::span<U>& other) const

@jwellbelove
Copy link
Contributor

jwellbelove commented Feb 11, 2023

Deep comparison was originally in std::span, but was removed.
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1085r2.md

Maybe some sort of overloaded etl::equal could be more relevant? (and make operator== shallow)

template <typename T1, size_t N1, typename T2, size_t N2>
etl::enable_if_t<etl::is_same_v<etl::remove_cv_t<T1>, etl::remove_cv_t<T2>>, bool>
  equal(const etl::span<T1, N1>& lhs, const etl::span<T2, N2>& rhs)
{
  return (lhs.empty() && rhs.empty()) ||
         ((lhs.begin() == rhs.begin()) && (lhs.size() == rhs.size())) ||
         etl::equal(lhs.begin(), lhs.end(), rhs.begin(), rhs.end());
}

@jwellbelove
Copy link
Contributor

There is the question of whether 'empty' spans should compare as equal.

int i;
etl::span<int> span1;
etl::span<int> span2(&i, &i);

bool equal = span1 == span2; // ?

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.

3 participants