-
Notifications
You must be signed in to change notification settings - Fork 68
-
Notifications
You must be signed in to change notification settings - Fork 68
Minor tasks from asynchronous review #742
Comments
|
About
|
I can live with 0 but I do find NULL clearer. Also, since NULL is |
In finalise.c :
|
In asmcomp/amd64/emit.mlp:
In runtime/amd64.S:
|
I would suggest to delay the following two suggestions for after the MVP (i.e. after merge of Multicore 5.00 in the main OCaml trunk), because they are not urgent at all.
I'd be happy to propose PRs for these two after the MVP merge. |
Thanks @xavierleroy. I've moved them to a separate issue #754. |
|
|
|
|
null_stk is |
stdlib/fiber.mli:
stdlib/fiber.ml:
|
|
|
I was pointed out there is a |
Agreed. The C callee saved registers need not be saved. In the previous design, the fiber stacks were managed on the OCaml heap. This is not something that we need immediately as stack realloc is a rare operation. I created an issue for tracking this feature for the future #772. |
|
@xavierleroy the only use of this is for the debug-only function I could duplicate the definitions of macros in The other option is to inline the function in Any other ideas to do this better? |
This is a fair review comment; the code is brittle and looks like it might have bad errors. However I will attempt a defence of the existing situation:
The defence is weak, however I (at least) couldn't see a fast fix as things would need to be tested and written carefully here. I have filed an issue to pick this up #805 |
This is a list of minor tasks from the asynchronous review phase. Major tasks will have their own issues. The multicore team members should feel free to start picking up these tasks and working on it. Please put your GitHub handle beside the task if you've picked up a task to avoid duplicating the work.
Replace with
The text was updated successfully, but these errors were encountered: