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

Improve Texp_record constructor representation #516

Merged
merged 15 commits into from Jul 12, 2016

Conversation

Projects
None yet
5 participants
@chambart
Copy link
Contributor

commented Mar 18, 2016

Accessing record information in the typedtree is not very practical, and the information about the type of kept fields (when using the { record with label = exp } construct) is available.

I propose to replace it by:

  | Texp_record of
      record_label_definition array *
      Types.label_description array *
      Types.record_representation *
       expression option

and record_label_definition =
  | Kept of Types.type_expr
  | Overridden of Longident.t loc * expression

This allows to easily access a particular field information (instead of searching for a field with the right field position).

The main motivation for this change is to provide the type information to translcore to propagate it to the lambda. In particular, the PR #336 would make use of it. I had a previous version that needed to do some unification after the type checking, but this version is a lot cleaner.

Notice that even if we lost the order of the fields in the parsetree, this does not affect the evaluation order. I added a test to verify that.

Notice that the evaluation order for record update depend on the number of fields updated:

  • if there are less than (size - 3) / 2 updated fields then the evaluation order is from the last index to the first
  • otherwise it is the opposite

Would anyone object if I changed the order to avoid this strangeness ?

@damiendoligez damiendoligez added this to the 4.04 milestone Mar 31, 2016

@chambart

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2016

Merged recent trunk and fix propagation of record update type informations

@chambart chambart force-pushed the chambart:improve_texp_record_representation branch from 5914078 to 455a4b5 Jul 7, 2016

@chambart

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2016

Re-rebased to the current trunk.

Also use the type information to populate makeblock kinds in case of record update and add some test to check that this effectively allow unboxing in case of record update.

This test was failing first due to a bug introduced by #538 . This contain the fix for it.

@chambart

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2016

Note also #538 changed the evaluation order of records.
The failure comes from the new evaluation order failing due to this change. If this is considered as irrelevant, I will make every record use the same evaluation order.

@chambart

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2016

@garrigue @lpw25 or @alainfrisch would you object to this change ?

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2016

I think it's a good change, but I did not review the patch itself.

record_label_definition array * Types.label_description array *

Why not (Types.label_description * record_label_definition) array? This makes an invariant explicit (the two arrays have the same length) and simplify the code that work on them.

Moreover, the comment in Typedtree is confusing since it leads to believe that only overridden fields are listed. Perhaps just adding a sentence about the fact that all fields of the record type are listed in the arrays would help.

@bluddy

This comment has been minimized.

Copy link

commented Jul 7, 2016

I would also recommend switching to inline records where there are more than a pair of parameters, for clarity.

@chambart

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2016

@alainfrisch, no real reason, it made some change a bit smaller, but this is marginal.

Thanks for the suggestions, I will try this.

@chambart chambart force-pushed the chambart:improve_texp_record_representation branch from 38948fe to 60a8e1d Jul 7, 2016

@chambart

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2016

Rererebased...

And applied @alainfrisch suggestions

@chambart

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2016

@bluddy in that case the name of the type of each field is probably sufficient to describe what is its concern, but that should probably go in another pr soon

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2016

The order I suggested (Types.label_description * record_label_definition) seems more natural (it follows the OCaml syntax).

Regarding (inline or not) record for the arguments: if you think this is a good idea, why not doing it in this PR? Typedtree is processed by external tools (reading .cmt files) so we should indeed aim at user-friendliness and stability (i.e. not change again the definition post 4.04).

@@ -181,6 +181,26 @@ module Stdlib = struct
| None -> default
| Some a -> f a
end

module Array = struct

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Jul 11, 2016

Contributor

This helper module is now useless, I think.

@chambart chambart force-pushed the chambart:improve_texp_record_representation branch from 7c28f45 to 32cbe38 Jul 11, 2016

If the type is { l1: t1; l2: t2 }, the expression
{ E0 with t2=P2 } is represented as
Texp_record
([| l1, Kept t1; l2 Override P2 |], representation,

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Jul 11, 2016

Contributor

This needs to be updated.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2016

Good to merge in my opinion (after fixing the doc).

@chambart

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2016

@alainfrisch thanks for the review.

@chambart chambart merged commit 1ae28b9 into ocaml:trunk Jul 12, 2016

2 checks passed

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

This comment has been minimized.

Copy link
Member

commented on 32cbe38 Nov 4, 2016

@chambart , why is #538 marked as a breaking change by this commit? Did you mean that #516 was a breaking change?

cc @damiendoligez: I think our release-time Changelog may be slightly wrong on that. (People will inspect the list of breaking changes)

This comment has been minimized.

Copy link
Contributor Author

replied Nov 4, 2016

I can't remember doing that voluntarily. This must have been some typo/merge problem.
Neither of those are expected to be breaking anything. Sorry for that.

This comment has been minimized.

Copy link
Member

replied Nov 7, 2016

I've reverted the change to GPR#538's status (commit 1158cfb).

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Merge pull request ocaml#516 from chambart/improve_texp_record_repres…
…entation

Improve Texp_record constructor representation

@bobzhang bobzhang referenced this pull request Sep 6, 2018

Open

to-do list for upgrade #3000

31 of 37 tasks complete
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.