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

Expose Spacetime.enabled #849

Merged
merged 2 commits into from Jan 10, 2017

Conversation

Projects
None yet
5 participants
@lpw25
Contributor

lpw25 commented Oct 11, 2016

This adds an enabled value to the Spacetime module so that people can turn on code for taking snapshots only when using a spacetime configured compiler.

I made the PR against 4.04 as it is a tiny low-risk change with obvious benefit.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Oct 26, 2016

Contributor

One risk I can see is that this PR adds a call to a new primitive at toplevel. In the context of js_of_ocaml, this means that as long as a Javascript replacement for the primitive is not provided, the program will fail at startup (even if the program does not refer to Spacetime if linked with -linkall). So I would rather suggest to wait for 4.05 -- users who really need it can redefine the external declaration locally -- or to expose the function without applying it at initialization time.

Contributor

alainfrisch commented Oct 26, 2016

One risk I can see is that this PR adds a call to a new primitive at toplevel. In the context of js_of_ocaml, this means that as long as a Javascript replacement for the primitive is not provided, the program will fail at startup (even if the program does not refer to Spacetime if linked with -linkall). So I would rather suggest to wait for 4.05 -- users who really need it can redefine the external declaration locally -- or to expose the function without applying it at initialization time.

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Oct 27, 2016

Contributor

We can just leave this until 4.05 I think. Thanks for spotting this @alainfrisch .

Contributor

mshinwell commented Oct 27, 2016

We can just leave this until 4.05 I think. Thanks for spotting this @alainfrisch .

@damiendoligez damiendoligez added this to the 4.05-or-later milestone Oct 27, 2016

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 9, 2016

Contributor

@hhugo Are you ok for us to merge this into trunk?

Contributor

mshinwell commented Dec 9, 2016

@hhugo Are you ok for us to merge this into trunk?

@hhugo

This comment has been minimized.

Show comment
Hide comment
@hhugo

hhugo Dec 10, 2016

Contributor

Fine with me.

Contributor

hhugo commented Dec 10, 2016

Fine with me.

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 12, 2016

Contributor

Approved but needs rebasing onto trunk

Contributor

mshinwell commented Dec 12, 2016

Approved but needs rebasing onto trunk

@mshinwell mshinwell added the approved label Dec 12, 2016

@lpw25 lpw25 changed the base branch from 4.04 to trunk Dec 12, 2016

@lpw25

This comment has been minimized.

Show comment
Hide comment
@lpw25

lpw25 Dec 12, 2016

Contributor

It's now based on trunk.

Contributor

lpw25 commented Dec 12, 2016

It's now based on trunk.

@lpw25 lpw25 merged commit 338049c into ocaml:trunk Jan 10, 2017

2 checks passed

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

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

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