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

Remove the Typed module #1258

Merged
merged 2 commits into from
Nov 12, 2024
Merged

Conversation

Halbaroth
Copy link
Collaborator

@Halbaroth Halbaroth commented Oct 9, 2024

This PR removes the output AST of the legacy typechecker. This AST was still used by the compilation of match expressions in Expr.

@Halbaroth Halbaroth marked this pull request as ready for review October 28, 2024 12:46
Copy link
Collaborator

@bclement-ocp bclement-ocp left a comment

Choose a reason for hiding this comment

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

Apologies, I thought I had already reviewed this one before my vacation but looks like I forgot to finish the review.

Overall looks good — I think the Expr module was a good boundary for the mk_match abstraction but I'm OK as is now that we have a single frontend.

Comment on lines 841 to 871
(* (* TO BE REMOVED *)
let debug_compile_match e cases res =
if Options.get_debug_adt () then begin
Printer.print_dbg ~flushed:false ~module_name:"Expr"
"compilation of: match %a with@ " print e;
let p_list_vars fmt l =
match l with
[] -> ()
| [e,_,_] -> Var.print fmt e
| (e,_,_) :: l ->
Format.fprintf fmt "(%a" Var.print e;
List.iter (fun (e,_,_) -> Format.fprintf fmt ", %a" Var.print e) l;
Format.fprintf fmt ")"
in
List.iter
(fun (p, v) ->
match p with
| Typed.Constr {name; args} ->
Printer.print_dbg ~flushed:false ~header:false
"| %a %a -> %a@ "
DE.Term.Const.print name
p_list_vars args
print v;
| Typed.Var x ->
Printer.print_dbg ~flushed:false ~header:false
"| %a -> %a@ " Var.print x print v;
)cases;
Printer.print_dbg ~header:false
"end@ result is: %a" print res;
end *)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
(* (* TO BE REMOVED *)
let debug_compile_match e cases res =
if Options.get_debug_adt () then begin
Printer.print_dbg ~flushed:false ~module_name:"Expr"
"compilation of: match %a with@ " print e;
let p_list_vars fmt l =
match l with
[] -> ()
| [e,_,_] -> Var.print fmt e
| (e,_,_) :: l ->
Format.fprintf fmt "(%a" Var.print e;
List.iter (fun (e,_,_) -> Format.fprintf fmt ", %a" Var.print e) l;
Format.fprintf fmt ")"
in
List.iter
(fun (p, v) ->
match p with
| Typed.Constr {name; args} ->
Printer.print_dbg ~flushed:false ~header:false
"| %a %a -> %a@ "
DE.Term.Const.print name
p_list_vars args
print v;
| Typed.Var x ->
Printer.print_dbg ~flushed:false ~header:false
"| %a -> %a@ " Var.print x print v;
)cases;
Printer.print_dbg ~header:false
"end@ result is: %a" print res;
end *)

something else for the definition of: %a"
DE.Ty.Const.print adt
end
module Match = struct
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: if using local modules for actual implementation work there is an abstraction boundary that should be ideally documented and at least typed.

Suggested change
module Match = struct
module Match : sig
type pat
val mk_pat : DE.term -> pat
val make : Expr.t -> (pat * Expr.t) list -> Expr.t
end = struct

@Halbaroth
Copy link
Collaborator Author

Overall looks good — I think the Expr module was a good boundary for the mk_match abstraction but I'm OK as is now that we have a single frontend.

I had the same concerns when I have done this modification. Expr is an internal AST of the library and Dolmen expressions will become the new input AST of the library. Moving this feature from Expr to Translate means we cannot use it directly on AE expressions as library developers, but we do not use it. Actually we could use it in models because it would be simpler to produce a match expression for model terms then compiles it. My opinion is that we do not need to focus on this detail now and we can move back a part of the code in Expr later if we want to produce match expressions from AE expressions. Keeping all the match compilation code in Translate now is just simpler in the current state of the code base.

@Halbaroth Halbaroth merged commit 671ffe6 into OCamlPro:next Nov 12, 2024
17 checks passed
@bclement-ocp
Copy link
Collaborator

I had the same concerns when I have done this modification. [..] My opinion is that we do not need to focus on this detail now and we can move back a part of the code in Expr later

This argument could have been made to avoid moving the code out of Expr in the first place ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants