-
Notifications
You must be signed in to change notification settings - Fork 224
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
Use size_t
for Janet types length and capacity and the API those types use
#1438
Conversation
@@ -1384,7 +1384,7 @@ | |||
/* Build grammar table */ | |||
const JanetKV *st = janet_unwrap_struct(peg); | |||
JanetTable *new_grammar = janet_table(2 * janet_struct_capacity(st)); | |||
for (int32_t i = 0; i < janet_struct_capacity(st); i++) { | |||
for (size_t i = 0; i < janet_struct_capacity(st); i++) { |
Check notice
Code scanning / CodeQL
Declaration hides variable Note
line 1293
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 believe this is noise
janet_fixarity(argc, 2); | ||
JanetBuffer *buffer = janet_getbuffer(argv, 0); | ||
double x = janet_getnumber(argv, 1); | ||
int64_t bitindex = (int64_t) x; | ||
int64_t byteindex = bitindex >> 3; | ||
int which_bit = bitindex & 7; | ||
if (bitindex != x || bitindex < 0 || byteindex >= buffer->count) | ||
if (bitindex != x || bitindex < 0 || (size_t) byteindex >= buffer->count) |
Check notice
Code scanning / CodeQL
Equality test on floating-point values Note
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.
byteindex
and bitindex
are not floats at the time of the comparison
On Alpine Linux I see these warnings:
|
There aren't any of them at the master branch. |
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.
General issue right now seems to be some code that won't work on 32 bit platforms and some dubious casts. Making arrays support more than 2^31 elements is not useful unless all user functions can actually access them.
Also some issues with marshalling where arrays might not be representable on all platforms. I'm not sure what the best solution/compromise there is - just disallow marshalling things with lengths that don't fit in 32 bits?
Realistically, I'm not sure I can merge this kind of thing without extensive testing since it will break lots of packages. I would want to go through all packages in pkg.Janet and see if they compile. I think many would probably compile with warnings and many will error out.
6fd6eef
to
ff1b324
Compare
…apacity` fields
e8b3d81
to
1baa045
Compare
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.
Some more minor changes to address possible integer overflow - also please run make format
Co-authored-by: John W Higgins <[email protected]>
So this does seem to work locally by itself, but as evidenced by the extensive changes to janet.h, I probably won't merge this as is - at the very least it would need to be a compile time option. This broke every C extension and program I tried it with completely, spork, jaylib, sqlite, some of personal projects, etc. at a source code leve - literally 0 percent compatibility. Not unexpected. I'm not opposed to breaking the ABI minorly or even majorly on version changes, but incremental upgrades should be fairly easy at a source code level. That said, if several other huge changes interface breaking come down the line that would also require a huge version bump (new GC / new C API, new compiler, rework of core data structures, etc.), this would be good to merge in. But as is, frankly not tenable. If useful to you though, I would recommend keeping synced with master. One idea - I wonder if defining ssize_t as jsize_t (which could be, by default int32_t) would solve the problem while allowing for an easy backwards compatibility switch. #ifdef JANET_USE_SIZE_T
typedef ssize_t jss_t;
typedef size_t js_t;
#else
typedef int32_t jss_t;
typedef uint32_t js_t;
#endif |
Use
size_t
for Janet types length and capacity and the API those types use, as discussed on Zulip