Use size_t for Janet types length and capacity and the API those types use
#1438
+622
−577
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -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
There was a problem hiding this comment.
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.
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. |
bakpakin
reviewed
Apr 16, 2024
There was a problem hiding this comment.
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.
GrayJack
force-pushed
the
use-size_t
branch
6 times, most recently
from
6fd6eef
to
ff1b324
Compare
…apacity` fields
GrayJack
force-pushed
the
use-size_t
branch
2 times, most recently
from
e8b3d81
to
1baa045
Compare
bakpakin
requested changes
Apr 21, 2024
bakpakin
reviewed
Apr 21, 2024
bakpakin
reviewed
Apr 21, 2024
wishdev
reviewed
May 10, 2024
Co-authored-by: John W Higgins <wishdev@gmail.com>
|
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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use
size_tfor Janet types length and capacity and the API those types use, as discussed on Zulip