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

Change compilation order of toplevel definitions. #1649

Merged
merged 4 commits into from Mar 9, 2018

Conversation

Projects
None yet
8 participants
@maranget
Copy link
Contributor

maranget commented Mar 7, 2018

This simple patch set changes the order of compilation of toplevel definitions, so that they are compiled by following input order. The change applies to the bytecode compiler at the lambda-code production phase. Notice that the native code compiler is unchnaged in tat aspect.

As a result:

  • Warnings emitted during lambfda-code production and resulting from the compilation of successive toplevel definition now appear by following definition order.

  • Native code and bytecode compiler console output get closer one to the other.

This is a "best effort" patch, it is not claimed that all warnings will appear by increasing location order, nor that the native and bytecode compilers always produce the same console output.

let ext_fields = rev_let_bound_idents pat_expr_list @ fields in
let body, size =
transl_structure loc ext_fields cc rootpath final_env rem
transl_structure loc ext_fields cc rootpath final_env rem (* Translate remainder of sruct second *)

This comment has been minimized.

@let-def

let-def Mar 7, 2018

Contributor

Typo sruct.

This comment has been minimized.

@maranget

maranget Mar 7, 2018

Author Contributor

Ok thanks.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Mar 7, 2018

Is getting warnings in an order closer to source code (resp. native compilation) the only goal here? If so, I'm wondering if we shouldn't directly target a full solution (just keep all warnings and emit them at the end, after applying any relevant sorting). Or is the incremental nature of warning outputs useful in some contexts?

@@ -22,61 +22,61 @@
(function a/1015 x/1016 (array.unsafe_set[gen] a/1015 0 x/1016))
(let
(eta_gen_len/1017 =
(function prim/1108 stub (array.length[gen] prim/1108))
(function prim/1065 stub (array.length[gen] prim/1065))

This comment has been minimized.

@gasche

gasche Mar 7, 2018

Member

I believe that these changes should not be here, that they come from a typo in a9aa39d. (cc @shindere)

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Mar 7, 2018

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Mar 7, 2018

I sent a PR to fix it: #1650. (If approved, this one should be rebased on top of it.)

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Mar 7, 2018

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Mar 7, 2018

@maranget the reference files for array_spec.ml were changed by #1650, could you rebase your PR on top of that?

The following should work from your New Order branch:

# refresh "trunk"
git checkout trunk
git pull

# go back to your branch
git checkout neworder

# rebase on top of the new trunk
git rebase trunk
# may complain about conflicts; in that case, just remove the conflicting files,
# run the testsuite again, and use `git rebase --continue`

# refresh the PR
git push --force <your personal remote name>

@maranget maranget force-pushed the maranget:neworder branch from cc39f58 to cf3290f Mar 7, 2018

@maranget

This comment has been minimized.

Copy link
Contributor Author

maranget commented Mar 7, 2018

@gasche Now rebased, seems to work.

@alainfrisch Well, a complete solution for getting all warnings in-order would be more involved than this simple solution. In other words, this PR is a contribution towards a better world, which does not cost much and, as far as I can see, has no drawback. One advantage is having one compilation console output reference file in place of two (or more, flambda being considered) for some tests in the test suite. See for instance #717.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Mar 7, 2018

Well, a complete solution for getting all warnings in-order would be more involved than this simple solution.

Fair enough, and I don't object to this PR. (But just buffering the warning outputs and sorting them before actually display should not be that complex.)

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Mar 7, 2018

(Side note: buffering warnings might also be a way to make warning-control attributes more robust. One could collect ad hoc warning scopes (relying on concrete source locations) and use them to filter warning. This would in particular allow to control warnings raised by the lexer/parser and those raised by the backend, which cannot currently be controlled by the attributes.)

@lpw25

This comment has been minimized.

Copy link
Contributor

lpw25 commented Mar 8, 2018

(Response to side-note: We'd have to be quite careful here, thanks to ppx locations can not always be trusted. For example, a ppx that tried to use attributes to avoid warnings in its generated code -- a not uncommon desire -- could turn off warnings for other parts of the code)

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Mar 8, 2018

(Response to the response: yes, you're right, the idea was too simplistic.)

maranget added some commits Mar 7, 2018

Control compilation order better.
This change applies to the compilation phase from typed tree to lambda code,
on the path used by the bytecode compiler.

    - Change transl_let (in translcore.ml) so that the body of a let
      construct and the bindindgs  can be evaluated in one order
      or the other.

    - Top to bottom order for (bytecode) compilation of top-level definitions
      (in translmod.ml).

As a result, warnings from different toplevel definitions emitted
during this compilation phase should should appear by increasing location.
As bytecode compilation order has changed some reference logs that co…
…ntain

some warnings are changed. Observe that warnings are now in-order.

@maranget maranget force-pushed the maranget:neworder branch from cf3290f to bb222ec Mar 8, 2018

maranget added a commit to maranget/ocaml that referenced this pull request Mar 8, 2018

maranget added a commit to maranget/ocaml that referenced this pull request Mar 9, 2018

Temporary (if ocaml#1649 merged) modification of test test/basic/patm…
…atch.ml.

Also change line numbers in compiler reference files.
@damiendoligez
Copy link
Member

damiendoligez left a comment

The code looks good. @maranget will merge after checking whether flambda also needs to be changed to match this compilation order.

@maranget maranget merged commit 965dd7d into ocaml:trunk Mar 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@trefis

This comment has been minimized.

Copy link
Contributor

trefis commented Mar 9, 2018

It doesn't look like the flambda backend was changed.
Why change the order of bytecode to make it consistent with clambda but not change flambda to make it consistent with the other two?

@maranget

This comment has been minimized.

Copy link
Contributor Author

maranget commented Mar 9, 2018

@trefis

This comment has been minimized.

Copy link
Contributor

trefis commented Mar 9, 2018

Ah indeed, you are correct.
Thank you!

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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.