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.

Sign up for GitHub

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

feat: remove Belt dependency from Stdlib #796

Merged
merged 4 commits into from
Oct 24, 2023
Merged

feat: remove Belt dependency from Stdlib #796

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
36c8d42
feat: remove `Belt` dependency from Stdlib
anmonteiro Oct 19, 2023
971ac69
chore: add changelog entry
anmonteiro Oct 20, 2023
02080e4
fix: test
anmonteiro Oct 20, 2023
bd8ff78
fix runtime tests
anmonteiro Oct 20, 2023
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
7 changes: 7 additions & 0 deletions Changes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
Unreleased
---------------

- Remove `Belt` as a dependency of `Stdlib`
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change. Most projects use Dune default config for implicit transitive deps, so they will have to add melange.belt to the list of libraries after upgrading. See melange-re/melange-re.github.io#140.

Copy link
Member Author

Choose a reason for hiding this comment

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

will mark it as such. thanks!

([#796](https://github.com/melange-re/melange/pull/796))


2.1.0 2023-10-22
---------------

Expand Down
2 changes: 1 addition & 1 deletion jscomp/others/belt_Result.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
* along with this program; if not, write to the Free Software
* Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. *)

type ('a, 'b) t = Ok of 'a | Error of 'b
type ('a, 'b) t = ('a, 'b) Stdlib.result = Ok of 'a | Error of 'b

let getExn = function Ok x -> x | Error _ -> raise Not_found

Expand Down
2 changes: 1 addition & 1 deletion jscomp/others/belt_Result.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
]}
*)

type ('a, 'b) t = Ok of 'a | Error of 'b
type ('a, 'b) t = ('a, 'b) Stdlib.result = Ok of 'a | Error of 'b

val getExn : ('a, 'b) t -> 'a
(**
Expand Down
2 changes: 1 addition & 1 deletion jscomp/others/dune
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
(public_name melange.belt)
(preprocess
(pps melange.ppx -unsafe))
(libraries melange.js)
(libraries melange)
(modules
:standard
\
Expand Down
69 changes: 0 additions & 69 deletions jscomp/stdlib/camlinternalOO.cppo.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,26 +96,14 @@ let public_method_label s : tag =
(**** Sparse array ****)

module Vars =
#ifdef BS
Belt.Map.String
Copy link
Member

Choose a reason for hiding this comment

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

Users of the standard library will lose this "optimization" to not use a functor, do we want to inline Belt.Map.String perhaps?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a very good question, that I hope I can answer sufficiently:

  • I think short term the answer is "we don't care" because the benefits (removing belt as a dep) justify it, and the code becomes the same as the upstream stdlib
    • even if it did, I don't think the impact would be important -- and Melange users barely use OCaml objects anyway.
  • longer term we want to add continuous benchmarks to melange, which we've added to the next quarter roadmap. Hopefully we can measure the impact of this change retroactively then.

Copy link
Member

Choose a reason for hiding this comment

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

and Melange users barely use OCaml objects anyway.

Not sure I get this part, could you elaborate? How are objects related to the discussion about larger bundles when using functors?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see, this is the camlinternalOO module 🤦

#else
Map.Make(struct type t = string let compare (x:t) y = compare x y end)
#endif
type vars = int Vars.t

module Meths =
#ifdef BS
Belt.Map.String
#else
Map.Make(struct type t = string let compare (x:t) y = compare x y end)
#endif
type meths = label Meths.t
module Labs =
#ifdef BS
Belt.Map.Int
#else
Map.Make(struct type t = label let compare (x:t) y = compare x y end)
#endif
type labs = bool Labs.t

(* The compiler assumes that the first field of this structure is [size]. *)
Expand Down Expand Up @@ -196,36 +184,21 @@ let new_method table =
index

let get_method_label table name =
#ifdef BS
match Js.undefinedToOption (Meths.getUndefined table.methods_by_name name)
with
| Some x -> x
| None ->
let label = new_method table in
table.methods_by_name <- Meths.set table.methods_by_name name label;
table.methods_by_label <- Labs.set table.methods_by_label label true;
label
#else
try
Meths.find name table.methods_by_name
with Not_found ->
let label = new_method table in
table.methods_by_name <- Meths.add name label table.methods_by_name;
table.methods_by_label <- Labs.add label true table.methods_by_label;
label
#endif

let get_method_labels table names =
Array.map (get_method_label table) names

let set_method table label element =
incr method_count;
if
#ifdef BS
Labs.getExn table.methods_by_label label
#else
Labs.find label table.methods_by_label
#endif
then
put table label element
else
Expand All @@ -249,31 +222,12 @@ let narrow table vars virt_meths concr_meths =
table.vars, virt_meth_labs, vars)
:: table.previous_states;
table.vars <-
#ifdef BS
Vars.reduceU table.vars Vars.empty
(fun[@u] tvars lab info ->
if List.mem lab vars then Vars.set tvars lab info else tvars);
#else
Vars.fold
(fun lab info tvars ->
if List.mem lab vars then Vars.add lab info tvars else tvars)
table.vars Vars.empty;
#endif
let by_name = ref Meths.empty in
let by_label = ref Labs.empty in
#ifdef BS
List.iter2 (fun met label ->
by_name := Meths.set !by_name met label;
by_label :=
Labs.set !by_label label
(Labs.getWithDefault table.methods_by_label label true)
) concr_meths concr_meth_labs;
List.iter2
(fun met label ->
by_name := Meths.set !by_name met label;
by_label := Labs.set !by_label label false;
) virt_meths virt_meth_labs;
#else
List.iter2
(fun met label ->
by_name := Meths.add met label !by_name;
Expand All @@ -287,7 +241,6 @@ let narrow table vars virt_meths concr_meths =
by_name := Meths.add met label !by_name;
by_label := Labs.add label false !by_label)
virt_meths virt_meth_labs;
#endif
table.methods_by_name <- !by_name;
table.methods_by_label <- !by_label;
table.hidden_meths <-
Expand All @@ -304,11 +257,7 @@ let widen table =
table.previous_states <- List.tl table.previous_states;
table.vars <-
List.fold_left
#ifdef BS
(fun s v -> Vars.set s v (Vars.getExn table.vars v))
#else
(fun s v -> Vars.add v (Vars.find v table.vars) s)
#endif
saved_vars vars;
table.methods_by_name <- by_name;
table.methods_by_label <- by_label;
Expand All @@ -325,20 +274,11 @@ let new_slot table =
index

let new_variable table name =
#ifdef BS
match Js.undefinedToOption (Vars.getUndefined table.vars name : int Js.undefined) with
| Some x -> x
| None ->
let index = new_slot table in
if name <> "" then table.vars <- Vars.set table.vars name index ;
index
#else
try Vars.find name table.vars
with Not_found ->
let index = new_slot table in
if name <> "" then table.vars <- Vars.add name index table.vars;
index
#endif

let to_array arr =
if arr = Obj.magic 0 then [||] else arr
Expand All @@ -357,11 +297,7 @@ let new_methods_variables table meths vals =

let get_variable table name =
try
#ifdef BS
(Vars.getExn table.vars name)
#else
Vars.find name table.vars
#endif
with Not_found -> assert false

let get_variables table names =
Expand Down Expand Up @@ -389,13 +325,8 @@ let create_table public_methods =
Array.iteri
(fun i met ->
let lab = i*2+2 in
#ifdef BS
table.methods_by_name <- Meths.set table.methods_by_name met lab ;
table.methods_by_label <- Labs.set table.methods_by_label lab true )
#else
table.methods_by_name <- Meths.add met lab table.methods_by_name;
table.methods_by_label <- Labs.add lab true table.methods_by_label)
#endif
public_methods;
table

Expand Down
2 changes: 1 addition & 1 deletion jscomp/stdlib/dune
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
(modes melange)
(preprocess
(pps melange.ppx))
(libraries melange.js melange.belt)
(libraries melange.js)
(stdlib
(modules_before_stdlib CamlinternalFormatBasics CamlinternalAtomic)
(internal_modules Camlinternal*))
Expand Down
2 changes: 1 addition & 1 deletion jscomp/stdlib/stdlib.cppo.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ external decr : int ref -> unit = "%decr"

(* Result type *)

type ('a,'b) result = ('a, 'b) Belt.Result.t = Ok of 'a | Error of 'b
type ('a,'b) result = Ok of 'a | Error of 'b

(* String conversion functions *)

Expand Down
2 changes: 1 addition & 1 deletion jscomp/stdlib/stdlib.cppo.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1382,7 +1382,7 @@ external decr : int ref -> unit = "%decr"
(** {1 Result type} *)

(** @since 4.03 *)
type ('a,'b) result = ('a, 'b) Belt.Result.t = Ok of 'a | Error of 'b
type ('a,'b) result = Ok of 'a | Error of 'b

(** {1 Operations on format strings} *)

Expand Down
92 changes: 46 additions & 46 deletions jscomp/test/dist/jscomp/test/ocaml_re_test.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions jscomp/test/dune
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
(library
(name melange_runtime_tests)
(modes melange)
(libraries test_runner melange.dom)
(libraries test_runner melange.dom melange.belt)
(wrapped false)
(preprocess
(pps melange.ppx))
Expand All @@ -29,7 +29,7 @@
(library
(name melange_tests_re_res)
(modes melange)
(libraries test_runner melange.dom)
(libraries test_runner melange.dom melange.belt)
(wrapped false)
(preprocess
(pps melange.ppx reason-react-ppx))
Expand Down
4 changes: 2 additions & 2 deletions test/blackbox-tests/belt-and-runtime.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ Try commonjs first
> (melange.emit
> (target melange)
> (alias melange)
> (libraries melange)
> (libraries melange melange.belt)
> (emit_stdlib false)
> (module_systems commonjs))
> EOF
Expand All @@ -35,7 +35,7 @@ Now es6
> (melange.emit
> (target melange)
> (alias melange)
> (libraries melange)
> (libraries melange melange.belt)
> (emit_stdlib false)
> (module_systems es6))
> EOF
Expand Down