Closed hannesm closed 4 years ago
[@len 0]
is useful for the purpose of determining alignment/offset, so I don't understand why the rationale for disallowing it in cstruct
.
That being said, in this case I think we can indeed remove the type. :)
@cfcs the "only positive int literal" was introduced in https://github.com/mirage/ocaml-cstruct/pull/265 (fixing issue https://github.com/mirage/ocaml-cstruct/issues/264 by @laudecay), maybe @emillon and @avsm have an opinion whether 0 should be allowed or not in perspective of this issue.
I'm fine with lifting the check so that it's just >= 0. But maybe we can support this use case natively without [@len 0]
?
What does the code that checks alignment/offset would look like?
I suppose that that the uint8_t
is not meaningful here, so something like {empty: void}
could be supported, or even type msg_clipboard_req = |
can be supported in ppx_cstruct
.
@emillon : it looks like this:
let msg_type_size = function
| MSG_BUTTON -> Some sizeof_msg_button
| MSG_CLIPBOARD_REQ -> Some sizeof_msg_clipboard_req
[...]
I guess we could just replace the definition with:
let sizeof_msg_clipboard_req = 0
@emillon @talex5 Indeed this is just 0
in practice. In general the idea was that if the type got updated, we would still be doing the right thing. I think it would be ideal if cstruct
could handle 0
correctly liked it used to, since 1) it's valid C 2) it makes changing cstruct definitions less dangerous.
The uint8_t
is not meaningful here, but it's a direct translation from C, which makes it easier to check that whoever copy-pasted-and-modified-slightly the original definitions did so correctly.
now that the fix is in, can someone cut a new release?
Sorry to repeat @cfcs, but can we please cut a release soon?
with the recently released cstruct 5.1.0, it became more strict: in
[@len y]
expressions,y
must be a positive integer (see https://github.com/mirage/ocaml-cstruct/pull/265).in lib/formats.ml, there is one occurence of
[@len 0]
(/cc @cfcs whose commit introduced this code):I'm not sure what a good fix would be (taking into account that there is no empty record type (discussed in https://github.com/ocaml/ocaml/issues/7583, empty variant is implemented https://github.com/ocaml/ocaml/pull/1546). Can we get rid of the type
msg_clipboard_req
entirely?For now, I'll PR to opam-repository an upper bound of cstruct in mirage-qubes (0.8.0, the only release which includes the code above), see https://github.com/ocaml/opam-repository/pull/15400.