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

Remove unix/io_page.ml #34

Merged
merged 1 commit into from Jun 13, 2017
Merged

Remove unix/io_page.ml #34

merged 1 commit into from Jun 13, 2017

Conversation

yallop
Copy link
Member

@yallop yallop commented Jul 19, 2016

There are currently two almost-identical modules in the repository:

The first of these has fallen into disrepair; for example, it includes the external declaration:

external alloc_pages: int -> t = "caml_alloc_pages"

which no longer matches the signature of caml_alloc_pages, which now has two arguments.

A related problem is that the C code for caml_alloc_pages is built as part of the "unix" library (io-page.unix), but used in the "lib" library (io-page).

This PR removes the obsolete unix/io_page.ml module and moves the C code for caml_alloc_pages into the io-page library.

@talex5
Copy link
Contributor

talex5 commented Jul 19, 2016

I think the C code needs to stay in .unix because on Xen we have a different implementation (or at least, we compile it with different flags). Would be good to move that code here from mirage-platform too.

@yomimono
Copy link
Contributor

This seems like a step in the right direction. Coincidentally I was just looking at #19 , which goes even farther in proposing removal of io-page code.

@yallop
Copy link
Member Author

yallop commented Jul 19, 2016

Ok, I'll trim this PR down a bit to just remove the dead code.

@hannesm
Copy link
Member

hannesm commented Jul 19, 2016

@talex5 uhm, in general stub code from specific libraries should move away from mirage-platform, since it is a pain to remember updating the library x (e.g. cstruct), and than adjust the stubs in mirage-platform... (see also mirage/mirage-platform#124)

@talex5
Copy link
Contributor

talex5 commented Jul 19, 2016

@hannesm yes, I think that's what I said. Move the code from mirage-platform to io-page.

@hannesm
Copy link
Member

hannesm commented Jul 19, 2016

oh, sorry, misread.

@djs55
Copy link
Member

djs55 commented Jun 13, 2017

If I understand correctly then

  • everyone agrees that stubs should move into the library they belong to. However we haven't figured out how to build the Xen code properly (or at least I haven't)
  • the unix/io_page.ml{,i} appear to be unused and should be deleted as in this PR
  • the io_page.unix sub package is still needed and contains the Unix stubs so shouldn't be removed from the _oasis -- I'll see if I can adjust this patch.

lib/io_page is the maintained version.
@djs55 djs55 merged commit 4d6ef75 into mirage:master Jun 13, 2017
@yallop yallop deleted the remove-unix branch June 13, 2017 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants