runtime/io.c: remove the hook machinery for channel locking #12446
Conversation
| @@ -1094,17 +1094,17 @@ void caml_output_val(struct channel *chan, value v, value flags) | |||
| caml_stat_free(blk); | |||
| blk = nextblk; | |||
| } | |||
| Flush_if_unbuffered(chan); | |||
There was a problem hiding this comment.
By moving the flush to caml_output_value, you're changing the behavior of caml_output_val, which is exported (not CAML_INTERNAL) and used by the debugger. The debugger manages its own flushing, if I remember correctly, but other potential users may not.
There was a problem hiding this comment.
The previous code looks fairly strange -- it breaks the convention of all other users of Flush_if_unbuffered that it happens right before we unlock, which is a helpful way to ensure that it happens often enough (no output is forgotten) and yet not too often (we are done with our planned sequence of writes).
If I read things correctly:
caml_output_valis in fact defined in intext.h only when CAML_INTERNALS is set, so we don't need to worry about unknown callers- this call only makes a difference for unbuffered channels, and the debugger does not use unbuffered channels (and indeed, as you mention, has an explicit flush right after the call)
This gives me the impression that the change is a no-op that makes the code more regular and less surprising. (Even after the change, caml_output_val can call a GC through check_pending in io.c, which makes the implementation of safe_output_value in the debugger a bit suspicious, but I don't understand what this is supposed to do and am happy to leave it alone.)
There was a problem hiding this comment.
The mystery of the placement of this flush operation remains... But this should not block this PR. I fixed the conflicts to get a last round of testing.
gasche
force-pushed
the
io.c-remove-hooks
branch
from
dd7283a
to
0b20d4b
Compare
gasche
force-pushed
the
io.c-remove-hooks
branch
from
0b20d4b
to
8509200
Compare
|
Merged, thanks! |
|
Beware that there is a conflict with the io bigarray PR which needs to be fixed. |
|
On it, thanks! (This is a non-textual conflict, so git did not detect it, but the build on trunk is currently broken.) |
|
Hopefully 76c4617 fixes the build on trunk. |
This is an alternative to #12444: instead of documenting hooks that are now unused by the compiler distribution, we remove them completely.
cc @xavierleroy