close
Skip to content

feat(stdlib): Add Buffer.getChar#2262

Merged
ospencer merged 4 commits into
grain-lang:mainfrom
spotandjake:spotandjake-buffer-setChar
Jun 30, 2025
Merged

feat(stdlib): Add Buffer.getChar#2262
ospencer merged 4 commits into
grain-lang:mainfrom
spotandjake:spotandjake-buffer-setChar

Conversation

@spotandjake
Copy link
Copy Markdown
Member

This adds Buffer.setChar and Buffer.getChar to complement their bytes counterparts.

Note: We have to read the first byte before getChar so we can correctly do the bounds check similar to how its done in Bytes.getChar.

Note: I realized we missed a piece of documentation on Bytes.getChar so I added it here.

Comment thread stdlib/buffer.gr Outdated
Comment thread stdlib/buffer.gr
@unsafe
provide let getChar = (index, buffer) => {
use WasmI32.{ (+), (&), (+), (==), (>) }
checkIsIndexInBounds(index, 1, buffer)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this 1 if it's UTF-8?

Copy link
Copy Markdown
Member Author

@spotandjake spotandjake Apr 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Characters can be between 1 and 4 bytes, we need to ensure the first byte exists so we can check the char size on line 406 and we do an additional length check on line 408 with the actual length.

This is why getChar needs to operate on the bytes directly rather than just using Bytes.getChar like our other helpers.

Comment thread stdlib/buffer.gr
}

/**
* Gets the UTF-8 encoded character at the given byte index.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the operation is intended to get a character starting at a byte index then if you point your index in the middle of a UTF-8 character then what would the expectation be?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure it would either return a character in the case that the rest was a valid char (I can't recall if thats ever the case), but more likely MalformedUnicode.

Copy link
Copy Markdown
Member

@ospencer ospencer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm somewhat against these get/set APIs (not just the ones introduced in this PR, the existing ones too). I think it really encourages the wrong use of Buffers; they're really just meant for appending data, and they add a bunch of mental burden of "am I supposed to use add or set?". I'm more in favor of removing the existing ones rather than adding more. What are the use cases here?

Comment thread stdlib/buffer.gr Outdated
Comment thread stdlib/buffer.gr Outdated
@spotandjake
Copy link
Copy Markdown
Member Author

I'm somewhat against these get/set APIs (not just the ones introduced in this PR, the existing ones too). I think it really encourages the wrong use of Buffers; they're really just meant for appending data, and they add a bunch of mental burden of "am I supposed to use add or set?". I'm more in favor of removing the existing ones rather than adding more. What are the use cases here?

If we have any of the get and set commands I think these should exist to match. If we want to talk about removing them though I think its fair, most applications would possibly be better served by Bytes, and Bytes.resize.

@ospencer
Copy link
Copy Markdown
Member

We only have them for integers (so they're missing for everything else), and they landed in the initial implementation... I think we were just kinda caught up in that implementation and didn't audit the full API.

@spotandjake spotandjake force-pushed the spotandjake-buffer-setChar branch from 1ff67e8 to 48fd368 Compare June 2, 2025 01:15
@spotandjake spotandjake changed the title feat(stdlib): Add Buffer.setChar and Buffer.getChar feat(stdlib): Add Buffer.getChar Jun 2, 2025
@spotandjake
Copy link
Copy Markdown
Member Author

I removed Buffer.setChar as discussed at the community meeting and will follow up with a pr removing the other Buffer.set apis in a pr closer to 0.8.

@spotandjake spotandjake force-pushed the spotandjake-buffer-setChar branch from 48fd368 to 3ef3b66 Compare June 15, 2025 00:29
@ospencer ospencer added this pull request to the merge queue Jun 30, 2025
Merged via the queue into grain-lang:main with commit 95d03cb Jun 30, 2025
12 checks passed
@github-actions github-actions Bot mentioned this pull request Jun 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants