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.lastIndexOf #1372

Merged
merged 6 commits into from
Aug 4, 2022

Conversation

spotandjake
Copy link
Member

This pr adds String.lastIndexOf, it works the same as indexOf but instead of returning the first occurance of the search it returns the last.

Copy link
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 think what might be a bit better here is reusing the code from indexOf. The only difference between the two would be the break line in the loop.

You can turn indexOf into a helper function that takes an additional argument, maybe called findFirst, and then that if becomes

      if (Memory.compare(ptr, pptr, psize) == 0n) {
        result = idx
        if (findFirst) break
      }

And then you can call the helper for both functions:

export let indexOf = (search, string) => {
  indexOf(search, string, true)
}
export let lastIndexOf = (search, string) => {
  indexOf(search, string, false)
}

.gitignore Outdated Show resolved Hide resolved
Copy link
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 left some suggestions here—I switched up the logic a bit to avoid a Memory.compare when we're not at the start of a valid codepoint.

stdlib/string.gr Outdated Show resolved Hide resolved
stdlib/string.gr Show resolved Hide resolved
stdlib/string.gr Outdated Show resolved Hide resolved
stdlib/string.gr Outdated Show resolved Hide resolved
stdlib/string.gr Outdated Show resolved Hide resolved
stdlib/string.gr Outdated Show resolved Hide resolved
stdlib/string.gr Outdated Show resolved Hide resolved
stdlib/string.gr Outdated Show resolved Hide resolved
stdlib/string.gr Outdated Show resolved Hide resolved
stdlib/string.gr Outdated Show resolved Hide resolved
- Optimizes String.lastIndexOf todo checks only on whole codePoints
- Undo Changes To GitIgnore
stdlib/string.gr Show resolved Hide resolved
stdlib/string.gr Show resolved Hide resolved
stdlib/string.gr Outdated Show resolved Hide resolved
Co-authored-by: Oscar Spencer <oscar.spen@gmail.com>
Copy link
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.

Great work!

@ospencer
Copy link
Member

ospencer commented Aug 1, 2022

Looks like you need to regenerate the Graindocs.

Copy link
Member

@phated phated 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 holding off my review until #1389 lands because this PR contains a bunch of formatting that shouldn't be included in the PR

@spotandjake
Copy link
Member Author

Looks like you need to regenerate the Graindocs.

GrainDoc Seems To Be Up To Date

Co-authored-by: Blaine Bublitz <blaine.bublitz@gmail.com>
@spotandjake spotandjake requested a review from phated August 4, 2022 19:21
Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

Really cool! I like that you are diving into the low-level Grain stuff @spotandjake 🎉

@spotandjake
Copy link
Member Author

spotandjake commented Aug 4, 2022

Really cool! I like that you are diving into the low-level Grain stuff @spotandjake 🎉

Thank you, after working on my linker I have needed to work with a lot more low level grain stuff and realized it is actually quite friendly and easy to work with.

@phated phated merged commit b73d9bf into grain-lang:main Aug 4, 2022
@github-actions github-actions bot mentioned this pull request Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants