Introduce Asttypes.arrow_flag to encode labelled arguments by let-def · Pull Request #25 · ocaml/ocaml

let-def · 2014-03-28T11:54:23Z

This commit introduce the type arrow_flag in place of string to represent labelled arguments.

Previously, labelled arguments where encoded as a string tagging the left hand-side of an arrow type:
"" is the absence of label, "?ident" for optional arguments, "ident" for labelled arguments.

The arrow_flag variant now brings more structure to function arguments.
The purpose is two fold: offers a proper encoding of labels rather than relying on string processing, and ease further extensions of arrow lhs.

gasche · 2014-03-28T13:23:18Z

parsing/pprintast.ml

why is this part of the diff?

I left the auto-indenter on while editing, I'll remove indentation from the rest of the patch if you prefer.

It would make review easier indeed; while you're at it, could you also remove the bootstrapped files? They will make clashes when rebasing the patch.

gasche · 2014-03-28T13:45:19Z

I'm a bit worried about the change introducing bugs: the patch does not come with any tests, and labels are not adequately tested in the current testsuite (I just checked and it seems there is no occurence of labels in type expressions in the testsuite). Could you come up with additions to the testsuite that would more thoroughly check that labels are handled correctly?

The changes are generally simple, so manually checking correctness should be possible. However, the patch is quite long (more than 500 changed lines) so this is a tiring process; I only manually checked correctness of everything upto btypes.ml excluded (in alphabetic order).

I have a criticism on the name and structure of the label information. I find arrow_flag less informative than label as a name, and I have the impression that having Optional of string and Label of string be two distinct constructors may not correspond to the natural semantic of having a name plus a "kind" information, as maybe suggested by the case-factorizing matchings in the Pexp_fun case here or Abstract_wrong_label there.

What about the following representation instead?

type label = (label_kind * string) option (* keep the "label" type name *)
and label_kind = Simple | Optional (* ... and maybe more later *)

let-def · 2014-03-28T13:45:53Z

I updated the pull request as you suggested in the patch comments.

let-def · 2014-03-28T13:58:14Z

I am not convinced by the representation you suggest. In my extension, I plan to add implicit arguments, which would make a new kind of arrows.

This arrow_flag is a way to represent different kind of arrows: currently there are simple, optional and explicitly named arguments. Then later, maybe implicit ones.

So the purpose of arrow_flag is to carry the meaning of operationally behaving like a function, but receiving special treatments in the frontend. But there is no reason all those special treatments carry a label.

bobot · 2014-03-28T20:35:39Z

parsing/pprintast.ml

The case for Optional and Labelled are identical, could you factorize them in one function that will also contain the computation of simple_name and take the symbols "?" or "~" as argument?

gasche · 2014-04-04T08:00:54Z

I'm ready to accept the representation you defend (despite bobot's remark mirroring some of mine); in any case, the new code is an improvement over the old. However, I'm still not convinced that the patch does not introduce regressions; I think we need a testsuite for label stuff that covers the affected code.

bobot · 2014-04-04T16:33:14Z

Hum, I can't build this branch, the previous commit compile, this one doesn't (make world.opt amd64):

[...]
../boot/ocamlrun ../boot/ocamllex cvt_emit.mll
16 states, 280 transitions, table size 1216 bytes
../byterun/ocamlrun ../boot/ocamlc -I ../stdlib -c -w +32..39 -warn-error A -I ../utils -I ../parsing -I ../typing -I ../bytecomp -I ../asmcomp -I ../driver -I ../toplevel cvt_emit.ml
make[4]: *** [cvt_emit.cmo] Erreur de segmentation
make[4]: *** Destruction du fichier « cvt_emit.cmo »

I was writing the fix for my two remarks bobot/ocaml@4dc7e3b and bobot/ocaml@9f8b9a5 . I'm not sure all compile, but you can take them if you want.

@bobot

Contributed by @bobot

let-def · 2014-04-10T16:28:44Z

@bobot: Thanks, I will try to apply your patches
@gasche: Sure, I will write some tests. Though I am not sure what is the proper way to do, but I'll find out

gasche · 2014-04-10T19:39:45Z

The default way to write tests is to create a directory in testsuite/tests, copy the Makefile of some other directory in it that looks related in the way it does its checking, and write ml files. The code of foo.ml will be compiled, run, with the result (whatever gets in the standard output) stored in foo.ml.result and compared to a foo.ml.reference you provide with the tests. If it matches, the test passes, otherwise it fails.

If you want to test the desugaring of the asttree or typedtree, I recently considered that case myself, and something that could be done (I don't think there are tests that do exactly that right now) is to reuse the Makefile of a tests that feeds its input to the toplevel, and add the compilation flags -dsource and -dtypedtree to the toplevel arguments.

bobot · 2014-11-11T14:31:05Z

This PR have been discussed during the last developers' meeting, where I had the pleasure to be. To sum-up:

arrow_flag is a bad name: arg_kind seems better.
removing the optional value seems a good idea too.
Could @garrigue look at it?
In favor of integration.

garrigue · 2014-11-12T08:06:07Z

I've got a look at the discussion.
I see no problem on the idea, and if there were enough people supporting it at the meeting, I have no problem going in this direction. However, rather than applying the patch blindly, I would prefer to have a feeling of what happens, so I may end up redoing most of the work myself.

yallop · 2014-12-22T13:07:15Z

Corresponding mantis issue: 6367.

A similar patch has now been merged in trunk: 15848037

let-def · 2014-12-24T16:45:32Z

Thanks for the information!

Flambda2 to cmm translation

Mark caml_initialize_field and caml_atomic* non-allocating in cmmgen

@stack

Introduce mode variables, making Alloc_mode.t an abstract type. - Forces exported mode variables to Heap (in Ctype.closed_schema_rec) - Allows [@stack] annotations in function arguments (+Bootstrap)

@stack

Introduce mode variables, making Alloc_mode.t an abstract type. - Forces exported mode variables to Heap (in Ctype.closed_schema_rec) - Allows [@stack] annotations in function arguments (+Bootstrap)

173842c Merge flambda-backend changes ed7eba2 Remove leading space from LINE. (oxcaml/oxcaml#484) bd61170 Bump magic numbers (ocaml#5) c50c47d Add CI builds with local allocations enabled 1412792 Move local allocations support behind '-extension local' 6d8e42a Better tail call behaviour in caml_applyN c7dac3d Typemod: toplevel bindings escape even if no variables are bound 82d6c3e Several fixes for partial application and currying d05c70c Pprintast support for new local syntax e0e62fc Typecheck x |> f y as (f y x), not ((f y) x) d7e34ce Remove autogeneration of @ocaml.curry b9a0593 Port oxcaml/oxcaml#493 0a872d9 Code review fixes from oxcaml/oxcaml#491 6c168bb Remove local allocation counting 3c6e7f0 Code review fixes from oxcaml/oxcaml#478 bb97207 Rename Lambda.apply_position a7cb650 Quieten Makefile when runtime dep files are not present c656dc9 Merge flambda-backend changes 11b5424 Avoid printing double spaces in function argument lists 7751faa Restore locations to Typedtree.{pat,let}_bound_idents_full e450b6c add build_ocaml_compiler.sexp 0403bb3 Revert PR 9895 to continue installing VERSION b3447db Ensure new local attributes are namespaced properly 7f213fc Allow empty functions again 8f22ad8 Bugfix: ensure local domain state is initialised 80f54dd Bugfix for Selectgen with regions e8133a1 Fix external-external signature inclusion 9840051 Bootstrap d879f23 Merge remote-tracking branch 'jane/local-reviewed' into local-merge 94454f5 Use Local_store for the local allocations ref 54a164c Create fewer regions, according to typechecking (ocaml#59) 1c2479b Merge flambda-backend changes ce34678 Fix printing of modes in return types 91f2281 Hook mode variable solving into Btype.snapshot/backtrack 54e4b09 Move Alloc_mode and Value_mode to Btype ff4611e Merge flambda-backend changes ce62e45 Ensure allocations are initialised, even dead ones 6b6ec5a Fix the alloc.ml test on 32-bit builds 81e9879 Merge flambda-backend changes 40a7f89 Update repo URL for ocaml-jst, and rename script. 0454ee7 Add some new locally-allocating primitives (ocaml#57) 8acdda1 Reset the local stack pointer in exception handlers (ocaml#56) 8dafa98 Improve typing for (||) and (&&) (ocaml#55) 8c64754 Fix make_check_all_arches (ocaml#54) b50cd45 Allow arguments to primitives to be local even in tail position (ocaml#53) cad125d Fix modes from or-patterns (ocaml#50) 4efdb72 Fix tailcalls tests with inlining (ocaml#52) 4a795cb Flambda support (ocaml#49) 74722cb Add [@ocaml.principal] and [@ocaml.noprincipal] attributes, and use in oo.mli 6d7d3b8 Ensure that functions are evaluated after their arguments (flambda-backend ocaml#353) 89bda6b Keep Sys.opaque_identity in Cmm and Mach (port upstream PR 9412) a39126a Fix tailcalls within regions (ocaml#48) 4ac4cfd Fix stdlib manpages build 3a95f5e Merge flambda-backend changes efe80c9 Add jane/pull-flambda-patches script fca94c4 Register allocations for Omitted parameter closures (ocaml#47) 103b139 Remove various FIXMEs (ocaml#46) 62ba2c1 Bootstrap a0062ad Allow local allocations for various primitives (ocaml#43) 7a2165e Allow primitives to be poly-moded (ocaml#43) 2af3f55 Fix a flaky test by refactoring TypePairs (ocaml#10638) 58dd807 Bootstrap ee3be10 Fix modes in build_apply for partial applications fe73656 Tweak for evaluation order of labelled partial applications (ocaml#10653) 0527570 Fix caml_modify on local allocations (ocaml#40) e657e99 Relax modes for `as` patterns (ocaml#42) f815bf2 Add special mode handling for tuples in matches and let bindings (ocaml#38) 39f1211 Only take the upper bounds of modes associated with allocations (ocaml#37) aec6fde Interpret arrow types in "local positions" differently c4f3319 Bootstrap ff6fdad Add some missing regions 40d586d Bootstrap 66d8110 Switch to a system with 3 modes for values f2c5a85 Bugfix for Comballoc with local allocations. (ocaml#41) 83bcd09 Fix bug with root scanning during compaction (ocaml#39) 1b5ec83 Track modes in Lambda.lfunction and onwards (ocaml#33) f1e2e97 Port ocaml#10728 56703cd Port ocaml#10081 eb66785 Support local allocations in i386 and fix amd64 bug (ocaml#31) c936b19 Disallow local recursive non-functions (ocaml#30) c7a193a GC support for local allocations (ocaml#29) 8dd7270 Nonlocal fields (ocaml#28) e19a2f0 Bootstrap 694b9ac Add syntax to the parser for local allocations (ocaml#26) f183008 Lower initial stack size 918226f Allow local closure allocations (ocaml#27) 2552e7d Introduce mode variables (ocaml#25) bc41c99 Minor fixes for local allocations (ocaml#24) a2a4e60 Runtime and compiler support for more local allocations (ocaml#23) d030554 Typechecking for local allocations (ocaml#21) 9ee2332 Bugfix missing from ocaml#20 02c4cef Retain block-structured local regions until Mach. 86dbe1c amd64: Move stack realloc calls out-of-line 324d218 More typing modes and locking of environments a4080b8 Initial version of local allocation (unsafe) git-subtree-dir: ocaml git-subtree-split: 173842c

* Remove unused Env.Local_value_escapes * Propagate escaping_context to Env locks to hint about errors

@tail

0b0aefb Turn some partial application warnings into hints (ocaml#11338) (ocaml#30) 2caa9ee Add [@tail] and [@nontail] annotations on applications to control tailcalls (ocaml#31) 9fb218a Update `promote` target to use the `one` machinery (ocaml#28) b5ea912 Make empty types immediate bc08236 Add failing test of an empty type being immediate f2d439f Propagate escaping_context to Env locks to hint about errors (ocaml#25) 35569e1 Allow warning 68 to be controlled by attributes (ocaml#16) 28a6243 Allow type_argument to weaken return modes of expected function types (ocaml#24) cdc728f Fix 'make alldepend' in otherlibs/dynlink 7807d18 make alldepend 2d6af2f Merge flambda-backend changes git-subtree-dir: ocaml git-subtree-split: 0b0aefb

Bumps [bs-platform](https://github.com/rescript-lang/rescript-compiler) from 8.3.1 to 8.4.2. - [Release notes](https://github.com/rescript-lang/rescript-compiler/releases) - [Changelog](https://github.com/rescript-lang/rescript-compiler/blob/master/Changes.md) - [Commits](rescript-lang/rescript@8.3.1...8.4.2) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Remove documentation tab when status.json is missing * Add status indicators Co-authored-by: Thibaut <[email protected]>

gasche reviewed Mar 28, 2014
View reviewed changes

Introduce Asttypes.arrow_flag to encode labelled arguments

eb411a7

bobot reviewed Mar 28, 2014
View reviewed changes

let-def added 2 commits April 10, 2014 16:58

[Parsing] factorize print_label in label_x_expression_param

1e9353f

Contributed by @bobot

Moved Btype.extract_label to Btype.extract_arg

a1db8f5

let-def closed this Nov 15, 2014

let-def deleted the arrow-flag branch November 15, 2014 13:30

gasche mentioned this pull request Apr 29, 2015

Automatically insert source location #126

Open

Gbury pushed a commit to Gbury/ocaml that referenced this pull request Jul 18, 2019

Merge pull request ocaml#25 from Gbury/flambda2.0-stable-uncps

2548a50

Flambda2 to cmm translation

anmolsahoo25 pushed a commit to anmolsahoo25/ocaml that referenced this pull request Aug 25, 2020

Merge pull request ocaml#25 from ctk21/smg_no_alloc_cextcalls

bec67fc

Mark caml_initialize_field and caml_atomic* non-allocating in cmmgen

poechsel pushed a commit to poechsel/ocaml that referenced this pull request Jul 2, 2021

Amd64 emit tweaks (ocaml#25)

f7d2731

lukemaurer pushed a commit to lukemaurer/ocaml that referenced this pull request Jul 19, 2022

Propagate escaping_context to Env locks to hint about errors (ocaml#25)

f2d439f

* Remove unused Env.Local_value_escapes * Propagate escaping_context to Env locks to hint about errors

Conversation

let-def commented Mar 28, 2014

Uh oh!

gasche Mar 28, 2014

Choose a reason for hiding this comment

Uh oh!

let-def Mar 28, 2014

Choose a reason for hiding this comment

Uh oh!

gasche Mar 28, 2014

Choose a reason for hiding this comment

Uh oh!

gasche commented Mar 28, 2014

Uh oh!

let-def commented Mar 28, 2014

Uh oh!

let-def commented Mar 28, 2014

Uh oh!

bobot Mar 28, 2014

Choose a reason for hiding this comment

Uh oh!

gasche commented Apr 4, 2014

Uh oh!

bobot commented Apr 4, 2014

Uh oh!

let-def commented Apr 10, 2014

Uh oh!

gasche commented Apr 10, 2014

Uh oh!

bobot commented Nov 11, 2014

Uh oh!

garrigue commented Nov 12, 2014

Uh oh!

yallop commented Dec 22, 2014

Uh oh!

let-def commented Dec 24, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants