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

fix custom operations of ComplexL #549

Merged
merged 1 commit into from
Dec 22, 2017
Merged

fix custom operations of ComplexL #549

merged 1 commit into from
Dec 22, 2017

Conversation

fdopen
Copy link
Contributor

@fdopen fdopen commented Oct 7, 2017

The custom operations of ComplexL are broken:

#require "ctypes.top";;
let d1 = LDouble.of_float 1. ;;
let d2 = LDouble.of_float 2. ;;
let c1 = ComplexL.make d1 d2 ;;
let c2 = ComplexL.make d1 d1 ;;
c1 = c2;;
let s1 = Marshal.to_string c1 [] ;;
let s2 = Marshal.to_string c2 [] ;;
s1 = s2;;
let c1':ComplexL.t = Marshal.from_string s1 0 ;;
let c2':ComplexL.t = Marshal.from_string s2 0 ;;
Obj.size (Obj.magic c1) != Obj.size (Obj.magic c1');; (* scary *)

It's not captured by the test suite, because marshalling and comparison are both wrong.

And one question: Why is norm used inside ldouble_serialize https://github.com/ocamllabs/ocaml-ctypes/blob/f2108c7655fe2288ebbca98a12faad54ecc1e22e/src/ctypes/ldouble_stubs.c#L155, but not in ldouble_deserialize or ldouble_complex_(de)serialize https://github.com/ocamllabs/ocaml-ctypes/blob/f2108c7655fe2288ebbca98a12faad54ecc1e22e/src/ctypes/ldouble_stubs.c#L461?
OCaml doesn't normalize its simple floats during marshsalling, e.g. -0.0 is still -0.0 after serialization and subsequent deserialization.

cc @andrewray

@andrewray
Copy link
Contributor

Thanks for digging out this bug. My understanding of how serialization should be implemented has increased greatly...

Regarding norm - you are certainly right that it should be used consistently between LDouble and ComplexL. Whether it should be used at all I am less sure - it is perhaps sensible to at least normalize NaN (I am not sure serializing a signaling NaN, for example, is a good idea) - though I would be happy to follow the OCaml convention for floats here.

@yallop
Copy link
Owner

yallop commented Oct 17, 2017

Thanks for the finding and fixing this, @fdopen. It looks good to me; please feel free to merge when you're happy.

I'm not sure what's best to do about normalization, but following the OCaml standard library behaviour seems wise.

There also seem to be other differences around NaNs: e.g. it appears that LDouble.nan is a quiet NaN (built by the standard nanl function), whereas Pervasives.nan is a signalling NaN (built by Int64.float_of_bits). Mantis PR5038 has some related discussion.

@yallop yallop merged commit b8e816d into yallop:master Dec 22, 2017
@fdopen fdopen deleted the ldouble branch April 14, 2018 12:10
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.

3 participants