janestreet / sexplib0

Library containing the definition of S-expressions and some base converters
MIT License
18 stars 18 forks source link

Support for OCaml 5.00 #6

Closed kit-ty-kate closed 2 years ago

kit-ty-kate commented 2 years ago

sexplib0 currently fails to build with:

#=== ERROR while compiling sexplib0.v0.14.0 ===================================#
# context              2.2.0~alpha~dev | linux/x86_64 | ocaml-variants.5.00.0+trunk | file:///home/opam/opam-repository
# path                 ~/.opam/5.00/.opam-switch/build/sexplib0.v0.14.0
# command              ~/.opam/opam-init/hooks/sandbox.sh build dune build -p sexplib0 -j 71
# exit-code            1
# env-file             ~/.opam/log/sexplib0-19-1e6421.env
# output-file          ~/.opam/log/sexplib0-19-1e6421.out
### output ###
#       ocamlc src/.sexplib0.objs/byte/sexplib0__Sexp_conv.{cmo,cmt} (exit 2)
# (cd _build/default && /home/opam/.opam/5.00/bin/ocamlc.opt -w -40 -g -bin-annot -I src/.sexplib0.objs/byte -intf-suffix .ml -no-alias-deps -open Sexplib0__ -o src/.sexplib0.objs/byte/sexplib0__Sexp_conv.cmo -c -impl src/sexp_conv.ml)
# File "src/sexp_conv.ml", line 105, characters 15-31:
# 105 |       let id = Obj.extension_id
#                      ^^^^^^^^^^^^^^^^
# Error: Unbound value Obj.extension_id
#     ocamlopt src/.sexplib0.objs/native/sexplib0__Sexp_conv.{cmx,o} (exit 2)
# (cd _build/default && /home/opam/.opam/5.00/bin/ocamlopt.opt -w -40 -g -I src/.sexplib0.objs/byte -I src/.sexplib0.objs/native -intf-suffix .ml -no-alias-deps -open Sexplib0__ -o src/.sexplib0.objs/native/sexplib0__Sexp_conv.cmx -c -impl src/sexp_conv.ml)
# File "src/sexp_conv.ml", line 105, characters 15-31:
# 105 |       let id = Obj.extension_id
#                      ^^^^^^^^^^^^^^^^
# Error: Unbound value Obj.extension_id
xclerc commented 2 years ago

Tentative patch:

--- a/src/sexp_conv.ml
+++ b/src/sexp_conv.ml
@@ -90,17 +90,6 @@ module Exn_converter = struct

   module Exn_ids = Map.Make (Int)

-  module Obj = struct
-    module Extension_constructor = struct
-      [@@@ocaml.warning "-3"]
-
-      type t = extension_constructor
-
-      let id = Obj.extension_id
-      let of_val = Obj.extension_constructor
-    end
-  end
-
   module Registration = struct
     type t =
       { sexp_of_exn : exn -> Sexp.t
@@ -133,9 +122,7 @@ module Exn_converter = struct
     let id = Obj.Extension_constructor.id extension_constructor in
     let rec loop () =
       let old_exn_id_map = !exn_id_map in
-      let ephe = Ephemeron.K1.create () in
-      Ephemeron.K1.set_data ephe ({ sexp_of_exn; printexc } : Registration.t);
-      Ephemeron.K1.set_key ephe extension_constructor;
+      let ephe = Ephemeron.K1.make extension_constructor ({ sexp_of_exn; printexc } : Registration.t) in
       let new_exn_id_map = Exn_ids.add old_exn_id_map ~key:id ~data:ephe in
       (* This trick avoids mutexes and should be fairly efficient *)
       if !exn_id_map != old_exn_id_map
@@ -161,7 +148,7 @@ module Exn_converter = struct
     match Exn_ids.find id !exn_id_map with
     | exception Not_found -> None
     | ephe ->
-      (match Ephemeron.K1.get_data ephe with
+      (match Ephemeron.K1.query ephe (Obj.Extension_constructor.of_val exn) with
        | None -> None
        | Some { sexp_of_exn; printexc } ->
          (match for_printexc, printexc with
@@ -169,14 +156,6 @@ module Exn_converter = struct
           | true, false -> None))
   ;;

-  module For_unit_tests_only = struct
-    let size () =
-      Exn_ids.fold !exn_id_map ~init:0 ~f:(fun ~key:_ ~data:ephe acc ->
-        match Ephemeron.K1.get_data ephe with
-        | None -> acc
-        | Some _ -> acc + 1)
-    ;;
-  end
 end

 let sexp_of_exn_opt_for_printexc exn = Exn_converter.find_auto ~for_printexc:true exn
@@ -452,14 +431,6 @@ let () =
       , function
         | Stack.Empty -> Atom "Stack.Empty"
         | _ -> assert false )
-    ; ( [%extension_constructor Stream.Failure]
-      , function
-        | Stream.Failure -> Atom "Stream.Failure"
-        | _ -> assert false )
-    ; ( [%extension_constructor Stream.Error]
-      , function
-        | Stream.Error arg -> List [ Atom "Stream.Error"; Atom arg ]
-        | _ -> assert false )
     ; ( [%extension_constructor Sys.Break]
       , function
         | Sys.Break -> Atom "Sys.Break"
diff --git a/src/sexp_conv.mli b/src/sexp_conv.mli
index 5b1aa68..ae94f44 100644
--- a/src/sexp_conv.mli
+++ b/src/sexp_conv.mli
@@ -285,9 +285,6 @@ module Exn_converter : sig
     -> (exn -> Sexp.t)
     -> unit

-  module For_unit_tests_only : sig
-    val size : unit -> int
-  end
 end

 (**/**)
kit-ty-kate commented 2 years ago

Thanks a lot!!

I’ve added your patch to https://github.com/janestreet/sexplib0/pull/7 (+ a little simplification) just so people who need it can have an easy PR to use.

xclerc commented 2 years ago

Thanks; I will try to restore the For_unit_tests_only module.

kit-ty-kate commented 2 years ago

@xclerc David’s commit in #7 restores pre 5.0 compatibility. Does this look ok for you?