-
Notifications
You must be signed in to change notification settings - Fork 214
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
Add buffer/push-* sized int and float #1359
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-review comments.
src/core/binary.c
Outdated
uint8_t bytes[2]; | ||
} u; | ||
|
||
u.data = (uint16_t) janet_getinteger(argv, 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking this should be janet_getuinteger
which exists below janet_getinteger
in src/core/capi.c
but I was getting these warnings/errors on make valtest
:
cc -O2 -g -std=c99 -Wall -Wextra -Isrc/include -Isrc/conf -fvisibility=hidden -fPIC -c build/c/janet.c -o build/janet.o
src/core/binary.c: In function ‘cfun_binary_write_uint16’:
src/core/binary.c:86:25: warning: implicit declaration of function ‘janet_getuinteger’; did you mean ‘janet_getinteger’? [-Wimplicit-function-declaration]
86 | u.data = (uint16_t) janet_getuinteger(argv, 2);
| ^~~~~~~~~~~~~~~~~
| janet_getinteger
src/core/capi.c: At top level:
src/core/capi.c:296:10: error: conflicting types for ‘janet_getuinteger’; have ‘uint32_t(const Janet *, int32_t)’ {aka ‘unsigned int(const Janet *, int)’}
296 | uint32_t janet_getuinteger(const Janet *argv, int32_t n) {
| ^~~~~~~~~~~~~~~~~
src/core/binary.c:86:25: note: previous implicit declaration of ‘janet_getuinteger’ with type ‘int()’
86 | u.data = (uint16_t) janet_getuinteger(argv, 2);
| ^~~~~~~~~~~~~~~~~
make: *** [Makefile:223: build/janet.o] Error 1
src/core/binary.c
Outdated
uint8_t bytes[8]; | ||
} u; | ||
|
||
u.data = (uint64_t) janet_getuinteger64(argv, 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe there is a historical reason for this but I found that I couldn't write an 8 byte uint64_t
. This function checks for the unsigned integer range using JANET_INTMAX_DOUBLE
which is 9007199254740992.0
or hex 20000000000000
(7 bytes).
This should almost certainly go in the buffer module, but otherwise seems reasonable. The ffi module and peg module have some of this functionality but with other limitations. I would prefer if this were just folded into the buffer module as "binary" isn't particularly descriptive for a module name. |
#if JANET_LITTLE_ENDIAN | ||
return 1; | ||
#endif | ||
} else if (!janet_cstrcmp(order_kw, "native")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int/to-bytes
used nil
to represent native endianness. I felt :native
was more descriptive but I would understand the desire to maintain consistency with what already exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another path to consistency might be for int/to-bytes
to also support :native
?
I suspect many binary protocols need to encode/decode floats to/from big/little endian. Here is a first draft, should it be included in the standard library.
I am not tied to the
binary
module as I'm aware there existsbuffer/*
functions that are similar and it could possibly be bundled into the existingbuffer
module. I was concerned about the naming becausebuffer/push-word
already exists and the signature is different with no control over endianness. I thought it might be confusing to addbuffer/push-uint32
in that case - but maybe not.Additionally,
binary/write-float32
andbinary/write-float64
could potentially be removed if we implement something likemath/float32-bits
andmath/float64-bits
respectively for constructing sizeduint
s from the underlying bits and then lean onbinary/write-uint32
andbinary/write-uint64
to write to the buffer.I just wanted to start these discussions and fix any outstanding bugs that may be present.