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

Merge functions based on partiality rather than Parmatch.irrefutable #1195

Merged
merged 1 commit into from Sep 1, 2017

Conversation

lpw25
Copy link
Contributor

@lpw25 lpw25 commented Jun 8, 2017

translcore.ml merges together nested functions as long as the parameter is "fluid". This is what gives us a 2 parameter function for things like:

type r = R

let rr = fun R y -> 0

but the definition of fluid uses Parmatch.irrefutable which does not use type information. So things like:

type 'a t = T : int t | S : string t

let tt : int t -> _ -> _ = fun T y -> 0

still gives a function of 1 argument returning a function of 1 argument.

Since the partiality of the match (which does use type information) is already available this patch uses it in place of Parmatch.irrefutable. I think this is strictly more accurate and safe.

Note that there are optimizations in later passes (e.g. simplify) that also attempt to merge functions together. However, these can be quite fragile. For instance, the above case was not merged in 4.03, but this was accidentally fixed in 4.04 for ocamlopt by removing some debugging events. An example, which is not merged on trunk is bar below (unlike foo which is always merged):

type 'a foo = A of float * int | C of int * float;;

let foo = fun (A(_, x) | C(x, _)) y -> x + y;;

type 'a bar = A : float * int -> int bar | B : string bar | C : int * float -> int bar;;

let bar : int bar -> _ -> _ = fun (A(_, x) | C(x, _)) y -> x + y;;

@trefis
Copy link
Contributor

trefis commented Jul 7, 2017

Apart from what's mentioned in #1227 this seems like a strict improvement.

@trefis
Copy link
Contributor

trefis commented Jul 7, 2017

(of course it would be nice if @maranget could confirm)

Copy link
Contributor

@trefis trefis left a comment

Choose a reason for hiding this comment

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

I think this should be merged now that #1227 has been merged.

Copy link
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

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

I reviewed the change (as part of #1308) and they seem correct.

@gasche
Copy link
Member

gasche commented Sep 1, 2017

(I don't understand why the CI check for Changes entry passes, but I will assume that this does not need a Change entry, add the label, and merge anyway.)

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.

None yet

3 participants