chore: Cleanup offset constants in String module
#2237
+406
−316
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
spotandjake
requested review from
ospencer,
peblair,
jozanza,
phated,
alex-snezhko and
marcusroberts
as code owners
alex-snezhko
approved these changes
Feb 2, 2025
| * @returns `true` if the byte is a leading byte, `false` otherwise | ||
| */ | ||
| @unsafe | ||
| provide let isLeadingByte = byte => { |
There was a problem hiding this comment.
Was this intended to be used somewhere?
There was a problem hiding this comment.
This is used in string.gr so its just a tiny bit of cleanup I noticed while looking for constants and decided to address in this pr.
alex-snezhko
approved these changes
Feb 3, 2025
| WasmI32.store(arr + arrIdx, str, 8n) | ||
| arrIdx += 4n | ||
| Memory.copy(str + _STR_DATA_OFFSET, last, strSize) | ||
| WasmI32.store(arr + _ARRAY_DATA_OFFSET, str, arrIdx) |
There was a problem hiding this comment.
I flipped them as I figured we should be offsetting into the array for the item rather than moving the pointer, it doesn't really make a difference though.
| let str = " argument must be non-negative" | ||
| throw Exception.InvalidArgument(name ++ str) | ||
| use WasmI32.{ (<) } | ||
| let _num = convSimpleNumber(num, name ++ " argument must be an integer") |
There was a problem hiding this comment.
A note is that in the nominal case this is now less efficient due to needing to do this string allocation + concat every time rather than only in the bad case
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.
This pr works towards #703 I started with the
stringmodule, I also changed a few other modules to use the commonoffsetsmodule for their offsets, if we are good with the approach I am taking to this I will do the same to the other modules but I figure that might be best todo in a separate pr.Notes:
Offsetsmodule for consistency sake.