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

Add Box abstraction #1099

Closed
wants to merge 1 commit into from
Closed

Conversation

emillon
Copy link
Collaborator

@emillon emillon commented Oct 25, 2019

A box is something that runs before and after a Fmt.t. This ensures that the corresponding "open" and "close" part are properly balanced by always maintaining them together.

(This might be superseded by #1074)

A box is something that runs before and after a [Fmt.t]. This ensures
that the corresponding "open" and "close" part are properly balanced by
always maintaining them together.
@Julow
Copy link
Collaborator

Julow commented Oct 25, 2019

Note that we already use functional boxes (eg. hvbox : int -> t -> t) and the compose function can be easily implemented on t -> t.

Only the code formatting modules uses imperative open/close functions and this is not making it safe (a little safer but not safe) and requires extra Box.unsafe_ functions.

@emillon
Copy link
Collaborator Author

emillon commented Oct 25, 2019

I think that the main two benefits to this approach are:

  • Box.t is a smaller type than t -> t, so we can introspect these more easily
  • the unsafe API is explicit. it's easier to determine where the weird parts are compared to the existing API.

@emillon
Copy link
Collaborator Author

emillon commented Oct 25, 2019

(this is not for immediate merge and won't solve all our issues but it's a good starting point to discuss some improvements - the API can be made lighter as well)

@Julow
Copy link
Collaborator

Julow commented Oct 25, 2019

* the unsafe API is explicit. it's easier to determine where the weird parts are compared to the existing API.

I don't agree. The unsafe part of Box is used the way as the existing API. If it's about the name, we can more rename change open_*box into unsafe_open_*box, same for close_box.

I don't think we have an abstraction problem. The unsafe open/close functions are only used marginally and will be removed in the future.

@emillon
Copy link
Collaborator Author

emillon commented Oct 25, 2019

Marking Format.open_*box would be useful, but there are cases that are more subtle. For example consider this piece of code:

ocamlformat/src/Fmt_ast.ml

Lines 3672 to 3685 in fdbd2f5

if Option.is_some blk_a.pro then
{ blk_a with
pro=
Some
( Cmts.fmt_before c pmod_loc
$ hvbox 2 fmt_rator $ Option.call ~f:blk_a.pro )
; epi= Some epi }
else
{ blk_a with
opn= open_hvbox 2 $ blk_a.opn
; bdy=
Cmts.fmt_before c pmod_loc $ open_hvbox 2 $ fmt_rator $ blk_a.bdy
; cls= close_box $ blk_a.cls $ close_box
; epi= Some epi }

It does not contain a call to Format.open_*box, however it breaks the balance between opn and cls. Box is made for these cases: you can't make open/closed unbalanced with just the safe API.

Does this make sense?

@Julow
Copy link
Collaborator

Julow commented Oct 25, 2019

That makes sense. But I suggest to change this code to not use these functions instead. We tried before and did not succeed, though.

@emillon
Copy link
Collaborator Author

emillon commented Dec 18, 2019

We'll find something better, closing.

@emillon emillon closed this Dec 18, 2019
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.

3 participants