Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Feb 6, 2018

Jbuilder now supports them. However, given that we can't check that a module is a pure interfaces and that not respecting this rule could lead to hard to debug errors, the user is required to explicitly list such modules.

The syntax is as follow:

(library
 ((name foo)
  (modules_without_implementation (x y z))))

@ghost ghost requested a review from rgrinberg February 6, 2018 13:12
@kit-ty-kate
Copy link
Member

(modules_with_implementation (x y z))))

Isn't it more (modules_without_implementation (x y z)))) ?

@ghost
Copy link
Author

ghost commented Feb 6, 2018

Indeed... I updated the description

Copy link
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that this implementation accepts making the alias module "mli only" when (wrapped true) is set. Is that correct? If it is, what are the semantics of that?

in
let opaque =
if cm_kind = Cmi && not (Module.has_impl m) && ctx.version >= (4, 03, 0) then
Arg_spec.A "-opaque"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess modules without implementation cannot introduce cross module optimization, is that why we add it unconditionally? A bit unrelated but others have noted that this is would be desirable in situations where they prefer faster recompilation over better generated code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's to silence a warning from the compiler, for some reason it still looks for the cmx.

A bit unrelated but others have noted that this is would be desirable in situations where they refer faster recompilation over better generated code.

Indeed, we should add support for that at some point. Is -opaque itself enough BTW? Or do we need to hide the cmx files as well

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I've made a ticket for it #497

@ghost
Copy link
Author

ghost commented Feb 7, 2018

I noticed that this implementation accepts making the alias module "mli only" when (wrapped true) is set. Is that correct? If it is, what are the semantics of that?

Currerntly the scheme is as follow: for a library named foo:

  • if the library has no module Foo, generate one and use it as alias module
  • if the library has a module Foo, don't wrap this module and generate the alias module as Foo__

There is no point giving an implementation to Foo__ since the user is not supposed to know it even exist. Now that we support interface only modules, it just seems more natural to make it an interface only module. We might be able to do the same in the first case as well, though I'm not sure it will always work when the user does things such as let module M = Foo in ... or Make(Foo).

@rgrinberg rgrinberg mentioned this pull request Feb 7, 2018
@rgrinberg
Copy link
Member

Now that we support interface only modules, it just seems more natural to make it an interface only module

Yeah, I think that makes sense if it doesn't break anything.

Ok, looks good. Thanks for explaining.

@ghost
Copy link
Author

ghost commented Feb 8, 2018

BTW, I'm wondering if we even need to install foo__.cmi

@ghost ghost force-pushed the intf-only branch from e7e1add to f13d803 Compare February 8, 2018 10:12
@ghost ghost merged commit b383828 into master Feb 8, 2018
@ghost ghost deleted the intf-only branch February 15, 2018 18:45
directory can't see each other unless one of them depend on the
other (#472)

- Better support for mli/rei only modules (#490)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be #489

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, it's now fixed. Thanks

This pull request was closed.
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.

3 participants