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 compilation time for toplevel include(struct ... end : sig ... end) #832

Merged
merged 2 commits into from Dec 21, 2016

Conversation

Projects
None yet
3 participants
@alainfrisch
Contributor

alainfrisch commented Oct 3, 2016

This is intended to fix MPR#7357, which uses the natural way of specifying an inline signature for a unit without using an external .mli file.

The trick is similar to the one applied for compiling module X = (struct ... end : sig ... end). Identifiers of the inner structure are "lifted" as extra fields of the top-level structure.

I'd argue this special case is relevant since it corresponds to the natural way of restricting a unit's signature without specifying an external .mli file. For instance, the change would be beneficial to #217 or to people using that encoding manually.

The trick is currently not applied to include (struct .... end); it would not be difficult to do so, but there is no real reason to use that form anyway (except perhaps to simplify code generators, or to customize warnings locally in a section), and one might argue that simplifying that to just the inner structure should be done earlier in the compilation process.

A minor point: I'm not sure about the call to Translattribute.check_attribute_on_module. In the current version, attributes on the include structure item are considered, not attributes on the either of the two module expression (the Tmod_structure and Tmod_constraint nodes). This is similar to what was done for the Tstr_module cases (starting from 58d6da1), but it does not look right to me. @chambart Can you comment?

@bobzhang

This comment has been minimized.

Show comment
Hide comment
@bobzhang

bobzhang Oct 3, 2016

Member

thanks for the quick fix.
Note that include (struct .. end ) is still useful for local open in code generator, but it is not a high priority.

Member

bobzhang commented Oct 3, 2016

thanks for the quick fix.
Note that include (struct .. end ) is still useful for local open in code generator, but it is not a high priority.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Oct 7, 2016

Contributor

Does anyone want to review this?

Contributor

alainfrisch commented Oct 7, 2016

Does anyone want to review this?

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Dec 13, 2016

Contributor

@garrigue Do you want to review this?

@chambart Can you comment on the attribute stuff (I think it was introduced by you in the existing case, which I only mimic here)?

Contributor

alainfrisch commented Dec 13, 2016

@garrigue Do you want to review this?

@chambart Can you comment on the attribute stuff (I think it was introduced by you in the existing case, which I only mimic here)?

@garrigue

This comment has been minimized.

Show comment
Hide comment
@garrigue

garrigue Dec 13, 2016

Contributor

I'm not sure I'm the best qualified to review this (except for having introduced my share of bugs in this part of the compiler).
Anyway, the code is clean, compact and non-intrusive, so if it passes the testsuite I would be tempted to say this is fine.
The improvement is limited to a specific pattern, but I understand it is an important one, particularly for generated code.

Contributor

garrigue commented Dec 13, 2016

I'm not sure I'm the best qualified to review this (except for having introduced my share of bugs in this part of the compiler).
Anyway, the code is clean, compact and non-intrusive, so if it passes the testsuite I would be tempted to say this is fine.
The improvement is limited to a specific pattern, but I understand it is an important one, particularly for generated code.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Dec 13, 2016

Contributor

Thanks @garrigue. Waiting a few days before merging in case @chambart's wants to comment about the attributes stuff.

Contributor

alainfrisch commented Dec 13, 2016

Thanks @garrigue. Waiting a few days before merging in case @chambart's wants to comment about the attributes stuff.

alainfrisch added some commits Oct 3, 2016

Improve compilation time for toplevel include(struct ... end : sig ..…
…. end)

This is intended to fix MPR#7357, which uses the natural way
of specifying an inline signature for a unit without using an external
.mli file.

The trick is similar than the one applied for compiling

  module X = (struct ... end : sig ... end)

Identifiers of the inner structure are "lifted" as extra fields
to the top-level structure.
@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Dec 21, 2016

Contributor

Rebased and added a Changelog.

Contributor

alainfrisch commented Dec 21, 2016

Rebased and added a Changelog.

@alainfrisch alainfrisch merged commit 9dbcb1e into ocaml:trunk Dec 21, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@alainfrisch alainfrisch deleted the alainfrisch:fix_7357 branch Dec 21, 2016

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

Improve compilation time for toplevel include(struct ... end : sig ..…
…. end) (#832)

* Improve compilation time for toplevel include(struct ... end : sig ... end)

This is intended to fix MPR#7357, which uses the natural way
of specifying an inline signature for a unit without using an external
.mli file.

The trick is similar than the one applied for compiling

  module X = (struct ... end : sig ... end)

Identifiers of the inner structure are "lifted" as extra fields
to the top-level structure.

* Changelog.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment