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

Distinguish bytes and string in generated stub code #622

Merged
merged 1 commit into from
Dec 23, 2019
Merged

Distinguish bytes and string in generated stub code #622

merged 1 commit into from
Dec 23, 2019

Conversation

yallop
Copy link
Owner

@yallop yallop commented Dec 23, 2019

OCaml 4.10 defaults to the force-safe-string mode, so that the String_val macro returns const char *.

This PR updates ctypes to generate calls to Bytes_val rather than String_val for uses of ocaml_bytes, ensuring const-correctness and fixing problems such as the one reported here.

@yallop yallop merged commit 1fc1a15 into yallop:master Dec 23, 2019
@yallop yallop deleted the bytes-val branch December 23, 2019 22:08
@fdopen
Copy link
Contributor

fdopen commented Jan 22, 2020

Still insufficent in case of ocaml_bytes.

Bytes_val returns unsigned char *:
https://github.com/ocaml/ocaml/blob/646d30404e6b5fa0d49aea3860cbf4efe3910601/runtime/caml/mlvalues.h#L269

The corresponding c type for Ctypes.ocaml_bytes is char *:
https://github.com/ocamllabs/ocaml-ctypes/blob/4327af50c24dbfdf5f9ba9c22b8f432de290b50d/src/ctypes/ctypes_type_printing.ml#L77

And clang triggers a warning message by default for such casts:
https://clang.llvm.org/docs/DiagnosticsReference.html#wpointer-sign

We should it either use ptr uchar for ocaml_bytes in the snippet above - or cast the unsigned specifier away inside the CTYPES_PTR_OF_OCAML_BYTES macro. Then there is at least the possibility that no warning is triggered.

@yallop
Copy link
Owner Author

yallop commented Jan 23, 2020

We should it either use ptr uchar for ocaml_bytes in the snippet above

This seems like the best fix to me. #625 makes this change.

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.

2 participants