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

Check arity of primitives #2015

Merged
merged 1 commit into from Sep 3, 2018

Conversation

Projects
None yet
4 participants
@hhugo
Copy link
Contributor

commented Sep 3, 2018

alternative to #2014

Should close MPR7408 and MPR7846

"%loc_FILE", Loc Loc_FILE, 0;
"%loc_LINE", Loc Loc_LINE, 0;
"%loc_POS", Loc Loc_POS, 0;
"%loc_MODULE", Loc Loc_MODULE, 0;

This comment has been minimized.

Copy link
@nojb

nojb Sep 3, 2018

Contributor

The %loc_* primitives can be used with zero or one arguments.

@hhugo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2018

I've just push an alternative approach

@diml

This comment has been minimized.

Copy link
Member

commented Sep 3, 2018

Is Obj.magic f x y z still allowed after this patch?

@hhugo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2018

Yes, I think so. Over-application is handled elsewhere.

@diml

This comment has been minimized.

Copy link
Member

commented Sep 3, 2018

Ok. It might be worth adding a test for this. You should be able to add an expectation test to test the error case BTW.

@@ -671,7 +688,7 @@ let lambda_of_prim prim_name prim loc args arg_exps =
| Send_cache, [obj; meth; cache; pos] ->
Lsend(Cached, meth, obj, [cache; pos], loc)
| (Raise _ | Raise_with_backtrace
| Lazy_force | Loc _
| Lazy_force | Loc _ | Primitive _

This comment has been minimized.

Copy link
@nojb

nojb Sep 3, 2018

Contributor

Please add missing Comparison _ case here.

(Pbigarrayref(false, 2, Pbigarray_unknown, Pbigarray_unknown_layout));
"%caml_ba_ref_3",
((Pbigarrayref(false, 2, Pbigarray_unknown, Pbigarray_unknown_layout)),
3);

This comment has been minimized.

Copy link
@nojb

nojb Sep 3, 2018

Contributor

Please fix the indentation here (and below).

"%field0", Primitive ((Pfield 0), 1);
"%field1", Primitive ((Pfield 1), 1);
"%setfield0", Primitive ((Psetfield(0, Pointer, Assignment)), 2);
"%makeblock", Primitive ((Pmakeblock(0, Immutable, None)), 1);

This comment has been minimized.

Copy link
@hhugo

hhugo Sep 3, 2018

Author Contributor

I'm not sure about %makeblock

This comment has been minimized.

Copy link
@nojb

nojb Sep 3, 2018

Contributor

%makeblock is not used in the compiler, it was replaced by %makemutable in ce804de.

I guess it can be removed?

This comment has been minimized.

Copy link
@nojb

nojb Sep 3, 2018

Contributor

(But if we wanted to leave it, the arity is correct)

@hhugo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2018

One could also write a function form Lambda.primitive to int to share some arity (at the cost of not having the arity and the primitive next to each other. @nojb any thought ?

@nojb

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2018

One issue is that some values of Lambda.primitive do not have a fixed arity, e.g. Pmakeblock, so some special casing will be necessary.

@hhugo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2018

The last commit checks arity at the time of declaration.

| Lazy_force -> p.prim_arity = 1
| Loc _ -> p.prim_arity = 1 || p.prim_arity = 0
| Send | Send_self -> p.prim_arity = 2
| Send_cache -> p.prim_arity = 4

This comment has been minimized.

Copy link
@nojb

nojb Sep 3, 2018

Contributor

What do you think about writing this as a pattern matching on (prim, p.prim_arity) ?

This comment has been minimized.

Copy link
@hhugo

hhugo Sep 3, 2018

Author Contributor

I didn't do it because the last pattern would have to either catch all or enumerate all constructors.

This comment has been minimized.

Copy link
@nojb

nojb Sep 3, 2018

Contributor

OK, fair enough.

@nojb

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2018

The last commit checks arity at the time of declaration.

Nice!

I read the patch and I believe it is correct. Do you have any other changes planned? If not, I think this can be merged once the check-typo failures are fixed. Also, I think it is worth adding a Changes entry.

@hhugo hhugo force-pushed the hhugo:arity branch from 72bacf7 to f16a1a7 Sep 3, 2018

@hhugo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2018

No other plan for me. I've fixed typos, added an entry in the changelog, rebased & squashed commits.

Changes Outdated
@@ -410,6 +410,9 @@ Working version
an `open` statement for which the `.cmi` is not available.
(Nicolás Ojeda Bär, report by Jocelyn Sérot, review by Gabriel Scherer)

- GPR#2015: Check arity of primitives

This comment has been minimized.

Copy link
@nojb

nojb Sep 3, 2018

Contributor

Could you please mention here MPR#7408 and MPR#7846 ? Thanks!

This comment has been minimized.

Copy link
@nojb

nojb Sep 3, 2018

Contributor

(and also add a dot after "primitives").

@hhugo hhugo force-pushed the hhugo:arity branch from f16a1a7 to 48ac996 Sep 3, 2018

@nojb nojb merged commit 0396ca0 into ocaml:trunk Sep 3, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nojb

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2018

Thanks!

@hhugo hhugo deleted the hhugo:arity branch Sep 3, 2018


let f a b c = a + b + c

let _ : int = Obj.magic f None None None

This comment has been minimized.

Copy link
@gasche

gasche Sep 4, 2018

Member

Out of curiosity, what is this part of the test for?

This comment has been minimized.

Copy link
@nojb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.