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

Add all renamed symbols to unixsupport.h #11336

Merged
merged 4 commits into from Jul 5, 2022
Merged

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Jun 20, 2022

Small follow-on from #10926, spotted in ocsigen/lwt#953. Most of the functions declared in unixsupport.h which were renamed had compatibility macros added as part of the original PR, but a few got missed.

This PR tidies unixsupport.h slightly to put them in one place and adds #define for all of the symbols which were renamed.

The only other public header from the unix library is socketaddr.h which already has all of these handled.

otherlibs/unix/unixsupport.h Outdated Show resolved Hide resolved
@Octachron Octachron self-assigned this Jun 27, 2022
#define win_alloc_handle caml_win32_alloc_handle
#define win_alloc_socket caml_win32_alloc_socket
#define win_CRT_fd_of_filedescr caml_win32_CRT_fd_of_filedescr
#define win32_maperr caml_win32_maperr
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The win32_maperr macri is also defined at line 78.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch - in fact, that macro not being guarded on L78 in the same way as the others means that there was a renaming missing in socket_win32.c (presumably a rebase artefact during the original PR)

Copy link
Member

@Octachron Octachron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All compatibility macros are here. It does make sense to not break user code when possible.

However, reviewing this PR, I was left wondering if we should offer a mechanism for hiding those compatibility macros outside of defining CAML_BUILDING_UNIX variable (which sounds like a very internal setting).

@dra27 dra27 merged commit 1916810 into ocaml:trunk Jul 5, 2022
dra27 added a commit that referenced this pull request Jul 5, 2022
Add all renamed symbols to unixsupport.h

(cherry picked from commit 1916810)
@dra27 dra27 deleted the unix-header-compat branch July 5, 2022 09:13
@dra27
Copy link
Member Author

dra27 commented Jul 5, 2022

However, reviewing this PR, I was left wondering if we should offer a mechanism for hiding those compatibility macros outside of defining CAML_BUILDING_UNIX variable

I'd been thinking about this, too. My rationale at the moment is that the present changes are about changing the linking symbols in the library (i.e. we've fixed the .a files) and then removing from the headers is a separate fix. I was thinking of putting something together for 5.1 to deprecate those symbols but we should also learn the "lessons" of caml/compatibility.h so that deprecation needs a "story" for libraries which wish to target 4.x and 5.x.

(which sounds like a very internal setting).

Yes, definitely - there's no expectation on user code to do this, it's just to prevent our code from inadvertently using the old symbols.

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 this pull request may close these issues.

None yet

3 participants