array and array_ref operator== and operator!= don't short circuit

[dsharlet]

These operators could return as soon as falsifying elements are found.

[jiawen]

Oof, I just ran into this.

Not only do they not short circuit, they compare all elements, which can be slow and surprising without looking at the code.

I expected built-in operations on array_refs to be cheap, with slow operations deferred to the free functions nda::equal, nda::make_copy, etc. And we intentionally made it so that array does not have a copy constructor from array_ref.

[jiawen]

@fbleibel-g Would this break a lot of Google users?

[dsharlet]

@jiawen it sounds like you expect this to be a shallow equality? This issue is about a much more subtle issue: when doing a deep equality check, we don't short circuit upon finding a falsifying element.

Re: deep vs. shallow equality: I made array::operator== deep quality because that's what std::vector::operator== does: https://en.cppreference.com/w/cpp/container/vector/operator_cmp

Maybe it's reasonable to make array_ref::operator== a shallow equality check, while keeping array's the same for std::vector similarity?

[jiawen]

Oops. It was nearby but that TODO in the code didn't actually refer here.

Re: operator==, I think of nda::array <--> std::vector and nda::array_ref <--> std::span. For the former, I agree that the behavior should match. By the same token, maybe we should remove nda::array_ref::operator== and add something like nda::ptr_equals or the more cumbersome nda::ptr_and_shape_equals.

Notably, absl::Span talks about how unlike std::span, it provides operator== and is likely a design mistake.

To not break existing users, maybe add #define NDA_INCOMPATIBLE_DEPRECATED_ARRAY_REF_OP_EQUALS that defaults to false (but can be flipped back on). And like other projects, remove the option after a few years (or never).

[dsharlet]

I agree we should probably remove array_ref::operator==.

Maybe we just don't need to provide a helper for this? If it's just a.base() == b.base() && a.shape() == b.shape(). Looks like std::span doesn't provide anything like this either.

I think we should try just removing it. I don't think the usage of this library is big enough to worry about maintaining compatibility for such a likely obscure use case. We already have nda::equal so replacing operator== usage with that call is pretty easy.

[fbleibel-g]

Removing it SGTM. I can handle the Google side upgrade easily enough.

[jiawen]

Great. I'll put up a PR. There's no rush on this.