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

Make phys_equal more like native #1410

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

Make phys_equal more like native #1410

wants to merge 10 commits into from

Conversation

hhugo
Copy link
Member

@hhugo hhugo commented Feb 18, 2023

Fix #1372

@hhugo
Copy link
Member Author

hhugo commented Feb 20, 2023

This change revealed some issues with some js stubs what were returning -0 instead of 0.

@hhugo
Copy link
Member Author

hhugo commented Feb 22, 2023

@ty, is there any per difference between the two ? Is object.is available everywhere?

@TyOverby
Copy link
Collaborator

https://caniuse.com/mdn-javascript_builtins_object_is

It’s pretty widely supported!

@TyOverby
Copy link
Collaborator

As for performance impacts, I don't have a good intuition for if it would be faster or slower. A small microbenchmark that I just made seems to indicate that Object.is is faster

image

@hhugo
Copy link
Member Author

hhugo commented Feb 28, 2023

some other seems to show that === is faster (when inputs are ints and array, which should be the common case).

@hhugo hhugo force-pushed the sameness branch 2 times, most recently from eb52732 to dc2ff61 Compare February 28, 2023 15:23
@hhugo
Copy link
Member Author

hhugo commented Feb 28, 2023

I'm not really happy with the current implementation, mostly because the decision to use === or Object.is is syntactic.
@vouillon, any opinion ?

@hhugo
Copy link
Member Author

hhugo commented Oct 11, 2024

I'm not really happy with the current implementation, mostly because the decision to use === or Object.is is syntactic. @vouillon, any opinion ?

I've changed the implementation. We now keep track of whether Eq and Neq compare float or non-float values.

@hhugo hhugo changed the title WIP: Sameness Make phys_equal more like native Oct 11, 2024
@hhugo hhugo marked this pull request as ready for review October 11, 2024 08:03
@hhugo hhugo requested a review from TyOverby October 11, 2024 08:06
@vouillon
Copy link
Member

I'm not sure how much of a difference this PR makes. In most cases, the physical equality will still not behave the same. For instance, this will still return true with Js_of_ocaml:

# 0. == 0.;;
- : bool = false

@hhugo
Copy link
Member Author

hhugo commented Oct 12, 2024

I'm not sure how much of a difference this PR makes. In most cases, the physical equality will still not behave the same. For instance, this will still return true with Js_of_ocaml:

# 0. == 0.;;
- : bool = false

The most interesting bit is "fixing" phys_equal between 0. and -0..
The rest (whether two identical float are equal or not) can been seen as an implementation detail.

That said, looking at the documentation of ==, 0. == -0. is fine.

   On non-mutable types, the behavior of [( == )] is
   implementation-dependent; however, it is guaranteed that
   [e1 == e2] implies [compare e1 e2 = 0].

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.

Use Object.is instead of === to implement phys_equal
3 participants