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

add Bytes.indexOf and Text.indexOf builtins #4065

Merged
merged 2 commits into from
Jun 8, 2023
Merged

Conversation

stew
Copy link
Member

@stew stew commented Jun 2, 2023

fixes #4060

@stew stew requested review from pchiusano and dolio June 2, 2023 02:28
Copy link
Member

@pchiusano pchiusano left a comment

Choose a reason for hiding this comment

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

I'll let @dolio take a look, but seems good to me.

@lexi-lambda is there a similarly easy way to implement this for the rope type used in the JIT for Text and Bytes? For the Haskell implementation, @stew is just converting to a lazy text or bytestring and then calling the existing search function.

Copy link
Contributor

@dolio dolio left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I don't think it should be too difficult to implement one of the string search algorithms directly on the chunked sequences. KMP is probably easiest, because it operates on elements in order. But that's not what e.g. Text in Haskell is using (I'm not sure what it is; maybe Boyer-Moore, since it mentions O(m*n) uncommon cases). I'm not really up on what is considered the best string search algorithm these days.

@pchiusano
Copy link
Member

pchiusano commented Jun 8, 2023

Cool, we're going to hold off merging this until @SystemFw can get a barebones implementation in Racket, likely early next week.

@stew stew merged commit 7d60786 into unisonweb:trunk Jun 8, 2023
5 checks passed
@stew stew deleted the indexOf branch June 8, 2023 21:28
@pchiusano
Copy link
Member

@SystemFw I added these as todos to the must have list here, we'll address in a follow up. @stew was wanting these ASAP for some other work and merged it.

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.

Bytes.indexOf : Bytes -> Bytes -> Optional Nat
5 participants