janestreet / bin_prot

Binary protocol generator
MIT License
73 stars 21 forks source link

Ghgsat/cube #20

Closed berke closed 4 years ago

berke commented 6 years ago

Hi,

Here is some support for floating-point Array3 types (rank-3 arrays or "cubes" as we call them.) Some definitions required to use vec32/vec64 and mat32/mat64 with bin_io seem to be missing. For cubes I added some aliases, but overall there is redundancy and I'm not sure what to do with that.

ghost commented 6 years ago

Thanks, this looks good. We require commits to be signed, could you check the CONTRIBUTING.md file in the repository?

What redundancy are you thinking about BTW?

berke commented 6 years ago

Thanks, this looks good. We require commits to be signed, could you check the CONTRIBUTING.md file in the repository?

I just did, I'm clearing it with GHGSat and I'll get back to you.

What redundancy are you thinking about BTW?

For example vec, vec64 and float64_vec, mat, mat64 and float64_mat. In addition, I'm not sure why but I had to add cube32 and cube64 aliases in Size to make the syntax extension happy, but it's entirely possible I was doing something wrong.

  let bin_size_cube            = bin_size_len + bin_size_len + bin_size_len
  let bin_size_float32_cube    = bin_size_cube
  let bin_size_float64_cube    = bin_size_cube
  let bin_size_cube32          = bin_size_cube
  let bin_size_cube64          = bin_size_cube
ghost commented 6 years ago

Ah, I see. I suppose if you remove the bin_..._cube32 values from std.ml, you should be able to remove the other cube32 aliases in the various modules. Looking at mat and vec, the convention seems to be the following:

but we don't have xxx32 or xxx64, so we should do the same for cubes.

aalekseyev commented 4 years ago

This got superseded by #21, which in turn got closed.