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

Expose OCaml version in C headers #6406

Closed
vicuna opened this issue May 8, 2014 · 11 comments
Closed

Expose OCaml version in C headers #6406

vicuna opened this issue May 8, 2014 · 11 comments

Comments

@vicuna
Copy link

@vicuna vicuna commented May 8, 2014

Original bug ID: 6406
Reporter: @whitequark
Assigned to: @damiendoligez
Status: closed (set by @damiendoligez on 2014-07-21T20:40:05Z)
Resolution: fixed
Priority: normal
Severity: minor
Target version: 4.02.0+dev
Fixed in version: 4.02.0+dev
Category: runtime system and C interface
Tags: patch, junior_job
Monitored by: @gasche

Bug description

I was specifically bitten by this commit: 05100e5#diff-f940572c32e55a7e40e7f693deea4c7bR42

Which changed, as far as I understand, a documented public API in an incompatible way between minor versions. This is horrible on its own, but the fact that there is no way to detect OCaml version in the C code to disambiguate makes it also impossible to work around.

File attachments

@vicuna
Copy link
Author

@vicuna vicuna commented May 8, 2014

Comment author: @gasche

The documentation/specification
http://caml.inria.fr/pub/docs/manual-ocaml/intfc.html
says:

caml_alloc(n, t) returns a fresh block of size n with tag t.
If t is less than No_scan_tag, then the fields of the block
are initialized with a valid value in order to satisfy
the GC constraints.

Which "valid value" is used is not specified in the documentation, so I don't think it was correct to rely on a precise choice of initialization value in the first place.

Could you explain the ctypes use-case? I had a glimpse of the discussion but that was quite cryptic to someone not familiar with the implementation.

@vicuna
Copy link
Author

@vicuna vicuna commented May 8, 2014

Comment author: @whitequark

You do have a point. Still, the rest of my argument stands.

The idea is that an OCaml callback fills some parts of an Obj.t array, and leaves some intact. The C code would then detect filled-in parts.

@vicuna
Copy link
Author

@vicuna vicuna commented May 9, 2014

Comment author: @mshinwell

I agree with Gabriel. I'm not sure it's a good idea to rely on which particular initializer is used.

Exposing a version string seems sensible.

@vicuna
Copy link
Author

@vicuna vicuna commented May 9, 2014

Comment author: @whitequark

A string is nearly impossible to perform comparisons on; how about OCAML_MAJOR OCAML_MINOR OCAML_PATCH, and in addition OCAML_VERSION that has the previous concatenated. That's how I usually seen it done.

E.g. for 3.12:

OCAML_MAJOR = 3
OCAML_MINOR = 12
OCAML_PATCH = 00
OCAML_VERSION = 31200L

For 4.01.00:

OCAML_MAJOR = 4
OCAML_MINOR = 01
OCAML_PATCH = 00
OCAML_VERSION = 40100L

This way you have both conveniently separated version parts and a way to compare them... e.g. #if OCAML_VERSION >= 40200L

@vicuna
Copy link
Author

@vicuna vicuna commented May 16, 2014

Comment author: @damiendoligez

A good idea, but isn't it overkill to make it a long int? I doubt we'll ever reach OCaml version 400000.00.0

@vicuna
Copy link
Author

@vicuna vicuna commented May 16, 2014

Comment author: @whitequark

Hm, I think I cargo-culted the L suffix from the C-internal definitions, where you have conditions like "#if STDC_VERSION > 199900L". Not sure whether it's actually needed or it's some obscure leftover.

@vicuna
Copy link
Author

@vicuna vicuna commented Jun 19, 2014

Comment author: dinosaure

I try to complete this issue with: #72

@vicuna
Copy link
Author

@vicuna vicuna commented Jul 16, 2014

Comment author: @damiendoligez

I want to change a few things before I apply this patch or the github one.

Do you have a strong preference for the format of the OCAML_VERSION integer (i.e. decimal as here or binary as in the github patch)? I'd rather use decimal.

@vicuna
Copy link
Author

@vicuna vicuna commented Jul 16, 2014

Comment author: @whitequark

Definitely decimal. Versions are in decimal, after all, it's just a form of fixed point. Also, I've never ever seen version constants in hex.

@vicuna
Copy link
Author

@vicuna vicuna commented Jul 17, 2014

Comment author: dinosaure

It is done at your convenience. :)

@vicuna
Copy link
Author

@vicuna vicuna commented Jul 21, 2014

Comment author: @damiendoligez

Patch modified and applied in 4.02 (commit 15016) and trunk (commit 15017).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants