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

Fix Spacetime under Windows #1478

Merged
merged 5 commits into from Dec 8, 2017

Conversation

Projects
None yet
3 participants
@nojb
Contributor

nojb commented Nov 14, 2017

This PR adapts Spacetime so it can be used under Windows.

  • Adapt spacetime.c to Unicode changes, fix compilation under msvc, etc.
  • Use the right calling convention in amd64.S when calling caml_spacetime_c_to_ocaml
  • Make the necessary Spacetime-related modifications to amd64nt.asm (used by msvc)
  • Use binary mode to read profiles in raw_spacetime_lib

Thanks @mshinwell for help debugging!

@xavierleroy

The changes I'm competent to comment on (e.g. the #define C_ARG_ in amd64.S) look good to me. For the Spacetime-specific aspects you need @mshinwell 's blessing.

@mshinwell

OK for merging after minor changes.

Show outdated Hide outdated asmrun/spacetime.c
#else
#define strdup_os strdup
#define snprintf_os snprintf
#endif

This comment has been minimized.

@mshinwell

mshinwell Dec 6, 2017

Contributor

These definitions aren't Spacetime-specific. Can they go elsewhere (or should we be using different functions)?

@mshinwell

mshinwell Dec 6, 2017

Contributor

These definitions aren't Spacetime-specific. Can they go elsewhere (or should we be using different functions)?

This comment has been minimized.

@nojb

nojb Dec 7, 2017

Contributor

They are needed for Unicode support. They are only used in spacetime.c so I would rather keep them there.

@nojb

nojb Dec 7, 2017

Contributor

They are needed for Unicode support. They are only used in spacetime.c so I would rather keep them there.

@nojb

This comment has been minimized.

Show comment
Hide comment
@nojb

nojb Dec 7, 2017

Contributor

Thanks for the review @mshinwell! Amended as suggested. Once you give the go-ahead, I will rebase on trunk and merge.

Contributor

nojb commented Dec 7, 2017

Thanks for the review @mshinwell! Amended as suggested. Once you give the go-ahead, I will rebase on trunk and merge.

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 7, 2017

Contributor

OK for merging, however please split the initializer of filename_len onto a separate line. (I have always found the syntax you are currently using to be confusing.)

Contributor

mshinwell commented Dec 7, 2017

OK for merging, however please split the initializer of filename_len onto a separate line. (I have always found the syntax you are currently using to be confusing.)

@nojb nojb merged commit d73ff39 into ocaml:trunk Dec 8, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment