Skip to content
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

feat(stdlib): Add String.repeat to String module #2140

Merged
merged 7 commits into from
Aug 31, 2024

Conversation

spotandjake
Copy link
Member

@spotandjake spotandjake commented Aug 13, 2024

This pr adds String.repeat to the standard library.

Should this be called String.make? It would align better with the Array library and Array.make the behaviour is similar.

stdlib/string.gr Outdated Show resolved Hide resolved
stdlib/string.gr Outdated Show resolved Hide resolved
@ospencer
Copy link
Member

Should this be called String.make? It would align better with the Array library and Array.make the behaviour is similar.

I personally think that String.make would be a little confusing. I would expect a String.make to be composed of characters instead of substrings. I think String.repeat is very descriptive. I'd welcome a second opinion, though.

stdlib/string.gr Outdated
* @since v0.6.7
*/
@unsafe
provide let repeat = (string, count) => {
Copy link
Member

Choose a reason for hiding this comment

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

We should probably reverse the arguments to put the string last.

stdlib/string.gr Outdated
throw InvalidArgument("Invalid count value")
}
let mut rawCount = untagSimpleNumber(count)
if (WasmI32.eqz(rawCount)) return ""
Copy link
Member

Choose a reason for hiding this comment

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

I personally wouldn't have these eqz checks. They make the implementation more complex with little benefit (people are likely calling this function on non-empty strings with count > 0 a vast majority of the time). The code still produces the same result without them.

stdlib/string.gr Outdated
let mut stringPtr = WasmI32.fromGrain(string)
let byteLength = WasmI32.load(stringPtr, 4n)
if (WasmI32.eqz(byteLength)) return ""
stringPtr += 8n
Copy link
Member

Choose a reason for hiding this comment

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

Since we don't ever mutate this again, it's probably better to just make a new binding, let stringPtr = stringPtr + 8n.

stdlib/string.gr Outdated
let byteLength = WasmI32.load(stringPtr, 4n)
if (WasmI32.eqz(byteLength)) return ""
stringPtr += 8n
let strPtr = allocateString(byteLength * rawCount)
Copy link
Member

Choose a reason for hiding this comment

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

We should rename this to something more clear, this is too similar to stringPtr

@spotandjake
Copy link
Member Author

I applied those changes

stdlib/string.gr Outdated Show resolved Hide resolved
stdlib/string.gr Outdated Show resolved Hide resolved
@ospencer
Copy link
Member

@spotandjake Can you regen the docs?

@spotandjake
Copy link
Member Author

I pushed the updated graindoc

@ospencer ospencer added this pull request to the merge queue Aug 31, 2024
Merged via the queue into grain-lang:main with commit 6c33d08 Aug 31, 2024
12 checks passed
@github-actions github-actions bot mentioned this pull request Aug 31, 2024
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.

2 participants