Fix the types of C primitives and remove some that are unused #12686
Conversation
xavierleroy
changed the title
There was a problem hiding this comment.
Nice to see the end of the special case for the int64 primitives! One minor nit (possibly) in ints.c and some extra stuff may need to be done for the runtime_events primitives.
Is it possibly worth a one-line comment in the lexing and parsing changes to be explicit that the structs match the value representation, just because we don't do that elsewhere in the runtime (I think?)
runtime/runtime_events.c
Outdated
| @@ -418,15 +418,15 @@ static void stw_create_runtime_events( | |||
| caml_global_barrier(); | |||
| } | |||
|
|
|||
| CAMLprim value caml_runtime_events_start(void) { | |||
| CAMLprim value caml_runtime_events_start(value vunit) { | |||
There was a problem hiding this comment.
These three form part of the public API for runtime events, which is breaking one of the tests. Perhaps rename them to caml_ml_runtime_events_start (a la lots of the I/O primitives) and then either #define caml_runtime_events_start or an inline'd definition of caml_runtime_events_start?
There was a problem hiding this comment.
You're right, it's a good idea to separate the C API functions caml_runtime_events_start and the primitive caml_ml_runtime_events_start. I went one step further and made the C functions void -> void instead of void -> value, as void -> void is the natural C type for these functions. Let me know what you think about that.
There was a problem hiding this comment.
I was fairly sure I wouldn't need to suggest the change to change the return type to void, indeed 🙂
There was a problem hiding this comment.
I'm glad you thought of changing the return type to void!
…ause,resume}`
These are wrappers for `caml_runtime_events_{start,pause,resume}` but with
types appropriate for calling from the bytecode interpreter.
…imitives They are part of the C API, but not callable from the bytecode interpreter. Seize the opportunity to give them type `void f(void)` instead of `value f(void)`.
Its type prevents it from being called from the bytecode interpreter.
xavierleroy
force-pushed
the
primitive-cleanup
branch
from
6abc3a4
to
3227201
Compare
…ions They were introduced in ocaml#2146 to support unboxed 64-bit integer operations on 32-bit platforms, something that is no longer needed in OCaml 5 and was removed in ocaml#11904.
`caml_lex_engine`, `caml_new_lex_engine`, `caml_parse_engine` have strange types with pointers to structs as parameters. Give parameters the type `value` and make explicit the casts from `value` to pointer to struct of `value` in the function bodies.
xavierleroy
force-pushed
the
primitive-cleanup
branch
from
e47ee97
to
afadab4
Compare
|
Thanks for the PR, sorry for the slip-up. It looks all good to me too. |
|
The fix to |
|
Thanks for the review and the merge. I don't think the wrong |
In preparation for a reboot of #11763 prompted by #12660, this PR makes sure that all functions declared
CAMLprimhave the correct types to be callable from the bytecode interpreter.Some functions that are not callable from the bytecode interpreter are declared
CAMLexportinstead, or removed since they are no longer used. See the commit messages for more details.