Unify toploop.mli / opttoploop.mli

[AltGr]

This unifies both implementations of the toplevel under a common interface.

Similar Makefile hacks to those in place for dynlink are used: byte
and native implementations are in separate directories, while the
shared interface are in the parent and copied as needed by the build
rules.

This is a preliminary step to remove duplication as much as possible
in the two implementations, but should already be pretty useful for
libraries that use the Toploop interface.

[gasche]

Similar Makefile hacks to those in place for dynlink are used: byte
and native implementations are in separate directories, while the
shared interface are in the parent and copied as needed by the build
rules.

Naive question: could we define the interface in a Toplevel_sigs module (that would define one signature for each of the .mli we want to share), and then simply have .mli files of the form include Toplevel_sigs.Foo?

[ghost]

That would make sense if we planned to keep the opttop* modules. However, the end goal is to have a single set of modules and get rid of the opttop* ones.

[gasche]

That would make sense if we planned to keep the opttop* modules. However, the end goal is to have a single set of modules and get rid of the opttop* ones.

So, if I understand correctly, the Makefile blurb is here only for a transitory period, and will be un-complexified after the code has been factorized appropriately? The plan is that, in the end, there is a single module that uses a dynamic check/option to behave as either the bytecode or native toplevel?

[AltGr]

Yes, this is a first stage in merging as much as possible from the toplevel/byte and toplevel/native directories ;
I'm not happier with the Makefile blurbs than you seem to be 🤢 (actually I don't think I would have dared do anything like that if it was not for the "nice" example in compilerlibs/dynlink 🙄 ).

On the outside, having a unified interface should already be an improvement though. And quite a few upgrades of the bytecode toplevel are being ported to the native one as it goes, so that's a benefit as well.

[ghost]

The plan is that, in the end, there is a single module that uses a dynamic check/option to behave as either the bytecode or native toplevel?

Either dynamic tests, either a module with a different implementation in byte code and native. The first option is simpler to implement, but it means that you'd end up linking the native compilation backend in byte code programs using the toplevel API, which is slightly odd.

[ghost]

One question regarding the Trace module; IIUC we continue to have it only in bytecode. That means that we cannot write a program that refers to Trace and have this program build out of the box in native. The programmer would need to resort to optional compilation or virtual libraries or something of the sort to be able to make their program work in both byte code and native code if they want to use Trace in bytecode.

Another option would be to systematically have a Trace module, but have all the functions fail with failwith "Not implemented" in native. IIRC, this is similar with what we do for some unix functions on Windows. WDYT?

[AltGr]

Indeed, in the current state of the PR, the Trace module only exists when compiling to bytecode.

Another option would be to systematically have a Trace module, but have all the functions fail with failwith "Not implemented" in native. IIRC, this is similar with what we do for some unix functions on Windows. WDYT?

This is what I have been doing in the current version for the Topdirs.dir_*trace* functions, but it's true that I didn't consider uses of the Trace module directly. I can make it shared as well, with a dummy implementation for native ; it may depend on the consumers of this module, I don't mind either way.

As for the topdirs, I have a second version underway where common directives are shared and each implementation defines its own (trace, etc.) in addition.

[ghost]

This is what I have been doing in the current version for the Topdirs.dir_trace functions, but it's true that I didn't consider uses of the Trace module directly. I can make it shared as well, with a dummy implementation for native ; it may depend on the consumers of this module, I don't mind either way.

Ack. Let's make it shared with a dummy implementation for native; it's more uniform and it will make things easier for consumers.

As for the topdirs, I have a second version underway where common directives are shared and each implementation defines its own (trace, etc.) in addition.

Sounds good

[ghost]

We just had another look with @AltGr. Just need to fix CI issues and add a disclaimer in trace.mli to say that this feature is not available in native mode, and then it looks good to merge.

[ghost]

We need a changelog entry BTW. There is one visible change: modules of the opttoplevel library are renamed OptXXX -> XXX

[ghost]

All looking good from my point of view! @AltGr could you confirm you are happy with the current state of the PR? I'll merge if yes.

[AltGr]

Yep! Still some work to do to clean up the code duplication, but this is fine as a first step forward :)

[ghost]

Great, merging!