Skip to content

Extend polymorphic compare to support objects. #2657

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 51 additions & 3 deletions jscomp/runtime/caml_obj.ml
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,18 @@ let caml_update_dummy x y =

type 'a selector = 'a -> 'a -> 'a

module O = struct
external isArray : 'a -> bool = "Array.isArray" [@@bs.val]
type key = string
let for_in : (Obj.t -> (key -> unit) -> unit) [@bs] = [%bs.raw
{|function (o, foo) {
for (var x in o) { foo(x) }
}
|}]
external hasOwnProperty : key -> bool [@bs.meth] = "" [@@bs.val]
let hasOwnProperty (o: Obj.t) (key: key) : bool = (Obj.magic o)##hasOwnProperty(key)
external get_value : Obj.t -> key -> Obj.t = "%array_unsafe_get"
end

let unsafe_js_compare x y =
if x == y then 0 else
Expand Down Expand Up @@ -199,7 +211,9 @@ let rec caml_compare (a : Obj.t) (b : Obj.t) : int =
let len_a = Bs_obj.length a in
let len_b = Bs_obj.length b in
if len_a = len_b then
aux_same_length a b 0 len_a
if O.isArray(a)
then aux_same_length a b 0 len_a
else aux_obj_compare a b
else if len_a < len_b then
aux_length_a_short a b 0 len_a
else
Expand All @@ -223,6 +237,27 @@ and aux_length_b_short (a : Obj.t) (b : Obj.t) i short_length =
let res = caml_compare (Obj.field a i) (Obj.field b i) in
if res <> 0 then res
else aux_length_b_short a b (i+1) short_length
and aux_obj_compare (a: Obj.t) (b: Obj.t) =
let min_key_lhs = ref None in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can probably avoid the allocation here if we used Js.Nullable.t?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that would work, though this runtime library does not have access to Js.Nullable.
I have a straight js version in da928db which does that and simplifies a bunch of other things.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bobzhang what do you think?

let min_key_rhs = ref None in
let do_key (a, b, min_key) key =
if not (O.hasOwnProperty b key) ||
caml_compare (O.get_value a key) (O.get_value b key) > 0
then
match !min_key with
| None -> min_key := Some key
| Some mk ->
if key < mk then min_key := Some key in
let do_key_a = do_key (a, b, min_key_rhs) in
let do_key_b = do_key (b, a, min_key_lhs) in
O.for_in a do_key_a [@bs];
O.for_in b do_key_b [@bs];
let res = match !min_key_lhs, !min_key_rhs with
| None, None -> 0
| (Some _), None -> -1
| None, (Some _) -> 1
| (Some x), (Some y) -> compare x y in
res

type eq = Obj.t -> Obj.t -> bool

Expand Down Expand Up @@ -268,15 +303,28 @@ let rec caml_equal (a : Obj.t) (b : Obj.t) : bool =
let len_a = Bs_obj.length a in
let len_b = Bs_obj.length b in
if len_a = len_b then
aux_equal_length a b 0 len_a
if O.isArray(a)
then aux_equal_length a b 0 len_a
else aux_obj_equal a b
else false
and aux_equal_length (a : Obj.t) (b : Obj.t) i same_length =
if i = same_length then
true
else
caml_equal (Obj.field a i) (Obj.field b i)
&& aux_equal_length a b (i + 1) same_length

and aux_obj_equal (a: Obj.t) (b: Obj.t) =
let result = ref true in
let do_key_a key =
if not (O.hasOwnProperty b key)
then result := false in
let do_key_b key =
if not (O.hasOwnProperty a key) ||
not (caml_equal (O.get_value b key) (O.get_value a key))
then result := false in
O.for_in a do_key_a [@bs];
if !result then O.for_in b do_key_b [@bs];
!result

let caml_equal_null (x : Obj.t) (y : Obj.t Js.null) =
match Js.nullToOption y with
Expand Down
Loading