-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Constify C constructors, arrays, and flags tables (take 2) #12951
Conversation
This is not a bug fix but merely a second round of optimization, isn't it? If I am not mistaken, the changelog entry belongs to the "Working version" section. |
I'm afraid f87f3ed will non-trivially break Jane Street's core-unix; cf. core_unix/src/core_unix_stubs.c#L1331-L1340: #if OCAML_VERSION_MAJOR < 5
#define caml_unix_getsockopt_aux unix_getsockopt_aux
#define caml_unix_setsockopt_aux unix_setsockopt_aux
#endif
extern value caml_unix_getsockopt_aux(char *name, enum option_type ty,
int level, int option, value v_socket);
extern value caml_unix_setsockopt_aux(char *name, enum option_type ty,
int level, int option, value v_socket,
value v_status); It's worth an opam-grep / GitHub code search when de-prefixing a symbol (cf. #10926) |
Yes, the changes are independent from the previous PR. I'll move to the working version.
Nice catch! Thanks.
Thanks, I'll keep that in mind! I couldn't remember on which machine I had downloaded the whole opam archive… the GitHub code search is definitely handy. |
I think that's a good change - it is an internal detail, so we shouldn't necessarily feel that we can't improve things because that might break external users (after all, I renamed the functions in 5.0!). The issue before was that there was no clear workaround if those functions became static other than copying the code. What I would suggest doing if this goes in is then opening parallel PRs to fix those packages pre-emptively (as in #10926, which of course usefully lists the packages which were affected by the renaming of those functions already.) |
I have found two packages impacted by this change, and submitted pull-requests. One has already been merged.
Is there anything that could block this PR? |
Antonin Décimo (2024/05/28 03:49 -0700):
Is there anything that could block this PR?
Should we wait for janestreet/core_unix#10 to be
merged, maybe?
|
Seems backwards to have to wait for downstream projects to merge these kind of PRs, acting on faith that this one will eventually be merged… |
I ended up reviewign this and I believe that this PR is an improvement The last commit is not strictly speaking related to the PR, I think, As a consequence, I have just approved and will now merge. |
Constify more constructors, arrays, and flags tables in C code. Now these tables will go in the readonly segment, where they belong.
This PR accounts for missed opportunities from #12223. I believe I've missed these because I searched with regexes that were too strong (starting with
static
, usingint
but ignoringDWORD
, …).I've tentatively modified the changes entry under the 5.2 section.