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

std.mem.eql: make comparisons for zero-sized and non-sized types work #19968

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wooster0
Copy link
Contributor

@wooster0 wooster0 commented May 14, 2024

Fixes #19929

@Rexicon226
Copy link
Contributor

Maybe a bit of a strange idea, but what if the Undefined case returned, "undefined"?

std.mem.eql compares the contents of two slices for equality, and equating Undefined is impossible imo. Either "undefined" or false.

@wooster0
Copy link
Contributor Author

wooster0 commented May 14, 2024

Well if T is @TypeOf(undefined) then it doesn't compare what's in the slices in the first place and just looks at the length because it knows that the contents are all undefined, in this case all the same value anyway (in some sense at least). The same applies to @TypeOf(Null) as the type (no size available for type '@TypeOf(null)') so I've added a special case for that as well.
That, or I think std.mem.eql should simply not support comparing undefined. Maybe same for null.

A bit related to #15909

@Rexicon226
Copy link
Contributor

in this case all the same value anyway (in some sense at least).

This is what I disagree with. There might be some existing precedent that supports the argument that untyped undefined is equal to another untyped undefined, but I don’t know if it.

I agree it probably would be best we just don’t support that.

@rohlem
Copy link
Contributor

rohlem commented May 14, 2024

to make comparing enum{}s possible

I don't see a value in this particular function. enum{} is a non-instantiable type.
Control flow paths that encounter an instantiation of this type are by definition unreachable (= illegal behavior when reached). IMO coercions from undefined shouldn't be exempt from this.

@Rexicon226
Copy link
Contributor

My 2c after thinking about this for a bit:

  1. the Undefined one should totally be illegal, that is a really silly pattern that should never be written.
  2. This is std.mem.eql. I think that the @sizeOf(T) == 0 check was OK to have because we are checking memory equality here. The solution to std.mem.eql not working with array of types #19929 is a std.meta function for slice checking if that's something we want supported. We are reaching outside the scope of the purpose of this function with this PR.

@wooster0
Copy link
Contributor Author

I don't see a value in this particular function. enum{} is a non-instantiable type.

Me neither but the commit description is now outdated and enum{} is comparable without any special handling. Once enum{} becomes actually non-instantiable (it currently is instantiable) the two lines in test eql testing enum{} can simply be removed.

the Undefined one should totally be illegal, that is a really silly pattern that should never be written.

I'm going to wait for a decision from a core team member here before making any changes.

This is std.mem.eql. I think that the @sizeOf(T) == 0 check was OK to have because we are checking memory equality here.

Well in the case of type, comptime_int, and comptime_float it's comptime memory that's compared which is still memory.
But maybe this should instead be handled by std.meta.eql? Should std.mem.eql then error for type, comptime_int, and comptime_float to avoid confusing behavior?

Again, going to wait for a core team member's decision here.

lib/std/mem.zig Outdated Show resolved Hide resolved
@rohlem
Copy link
Contributor

rohlem commented May 15, 2024

Once enum{} becomes actually non-instantiable (it currently is instantiable) the two lines in test eql testing enum{} can simply be removed.

I guess I view the tests as also living documentation of expected/intended behavior.
On any compiler change failing tests might be seen as indication that the change disagrees with the language usage within std.
That's why I'd personally exclude cases we already intend to drop support for at the time of writing.
However, I can see how that might be too abstract and subjective of a view, so it's presumably fine either way.

The `if (@sizeof(T) == 0) return true;` check simply doesn't work for a number
of cases so that is removed and changed into `@sizeOf(T) != 0` and then
used in the `eqlBytes` check chain to make comparing `enum{}`s possible.

The rest special-cases for comptime-only types and undefined to make
those comparisons possible as well.

Fixes ziglang#19929
* some kinds of types require comptime calls to eql
* don't rely on compare equal for undefined
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.

std.mem.eql not working with array of types
5 participants