-
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
Avoid using _Bool in public headers for the sake of C++ compatibility #12787
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe a similar change needs to happen in the 5.1
branch as https://github.com/ocaml/ocaml/pull/12734/files#diff-92e5994819665dce8d9ae0342af2dbb1ff3cd11666eb080873f54c2cc04a5ca3R138 introduced a use of _Bool
as well.
This other occurrence is under a I checked and the current modification is the only occurrence of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Do we have tests in the testsuite that fail if the RET_FROM_C_CALL macro does not work as expected?
Even if the remaining |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
We test that the poll happens when it should. We do not test that no poll happens when it should not (and it would ask some effort to devise one). I'm a proponent of using bool inside the runtime, which sometimes leads to using _Bool inside internal headers. All the more as the situation will resolve itself in the long term with C23. A test that would be nice to have in the CI is that C++ compilers understand public OCaml headers. |
Headers must be compatible with C++ so they cannot refer to _Bool. We replace _Bool for Caml_state->action_pending with intnat where only the least-significant byte is taken into account.
Fixes: #12772. See discussion there regarding the choice for s390x.
Precheck passes: https://ci.inria.fr/ocaml/job/precheck/915/