Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backward compatibility for Lock/Unlock in runtime/caml/io.h #12792

Closed
rjbou opened this issue Nov 28, 2023 · 8 comments · Fixed by #12793
Closed

Backward compatibility for Lock/Unlock in runtime/caml/io.h #12792

rjbou opened this issue Nov 28, 2023 · 8 comments · Fixed by #12793

Comments

@rjbou
Copy link
Contributor

rjbou commented Nov 28, 2023

Channel locking was rewritten in #12446. It removed Lock and Unlock macros in favor to caml_channel_lock and caml_channel_unlock functions.

This makes use of those functions/macros conditioned by the OCaml version. Is it possible to have a Lock and Unlock functions to keep backward compatibility and avoid conditional on OCaml version?

@gasche
Copy link
Member

gasche commented Nov 28, 2023

I want to see a pointer to your code in the wild using these (internal) definitions first :-)

@rjbou
Copy link
Contributor Author

rjbou commented Nov 28, 2023

Of course :) Here it is.

I want to get the value of channel mode (is binary?), the C function exists for that, but it is not externalised (only setting binary mode is in stdlib). And I lock channels as in the documentation is it mentioned that channels must be locked before calling caml_channel_binary_mode.

@dra27
Copy link
Member

dra27 commented Nov 28, 2023

Removing the hook machinery was fine, but I wonder if we're going to discover pre-existing C code out there which uses the locking macros when the 5.2 dev cycle starts? I don't think we checked that aspect at the time?

@gasche
Copy link
Member

gasche commented Nov 28, 2023

I don't mind putting the macros back. The reasoning was that the macros are behind the CAML_INTERNALS macros so they are not supposed to be used outside the runtime, or at least we do not provide compatibility guarantees.

Note: opamWindows.c does use CAML_INTERNALS, and the list of reasons why it needs the macro is incomplete as it does not mention those Lock macros.)

(Should In_channel expose a function to tell if a channel is in binary or text mode?)

@xavierleroy
Copy link
Contributor

I think users of CAML_INTERNALS mode can shoulder the consequences of our changes.

#if OCAML_VERSION_MAJOR >= 5
bla bla
#else
bla
#endif 

@dra27
Copy link
Member

dra27 commented Nov 28, 2023

I don't mind putting the macros back. The reasoning was that the macros are behind the CAML_INTERNALS macros so they are not supposed to be used outside the runtime, or at least we do not provide compatibility guarantees.

Absolutely - just to be clear, I wasn't meaning that it was incorrect that they were removed, only that we may soon discover other users who were using channels from C, while hopefully knowing that that involves a potentially unstable interface.

(Should In_channel expose a function to tell if a channel is in binary or text mode?)

I think these would be a good idea, yes (both In_channel and Out_channel).

@gasche
Copy link
Member

gasche commented Nov 28, 2023

@xavierleroy I would support this approach (serves you right, external CAML_INTERNAL users!) if supporting the previous interface came at any cost, but adding a #define Lock caml_channel_lock for backwards-compatibility is really trivial and unlikely to cause trouble. Let's bend slightly to make our users life better.

@rjbou
Copy link
Contributor Author

rjbou commented Nov 28, 2023

I don't mind putting the macros back.

Sorry if there was a misunderstanding, i didn't mean neither to propose going back to the hook macros machinery. What I had in mind was something like having (other) functions named Lock/Unlock for backward compatibility (simple redirection) for example.

Note: opamWindows.c does use CAML_INTERNALS, and the list of reasons why it needs the macro is incomplete as it does not mention those Lock macros.)

Doc updated :) thanks for pointing that.

Should In_channel expose a function to tell if a channel is in binary or text mode?

But I'd be fully satisfied to have it in {In,Out}_channel modules! My main/first need is to retrieve the current mode.

The reasoning was that the macros are behind the CAML_INTERNALS macros so they are not supposed to be used outside the runtime, or at least we do not provide compatibility guarantees

I think users of CAML_INTERNALS mode can shoulder the consequences of our changes.

#if OCAML_VERSION_MAJOR >= 5
bla bla
#else
bla
#endif 

It's indeed the current status of our code. I agree that one answer to this issue is "use internals at your own risk", i mainly opened it to point the backward incompatibility, and check if something could be done for it (in c runtime or stdlib).

gasche added a commit to gasche/ocaml that referenced this issue Nov 28, 2023
fixes ocaml#12792

Reported-by: Raja Boujbel <raja.boujbel@ocamlpro.com>
gasche added a commit to gasche/ocaml that referenced this issue Nov 28, 2023
fixes ocaml#12792

Reported-by: Raja Boujbel <raja.boujbel@ocamlpro.com>
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 a pull request may close this issue.

4 participants