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

Refactor Reline::Unicode ed_ vi_ em_ split_by_width methods #720

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

tompng
Copy link
Member

@tompng tompng commented Jun 9, 2024

Depend on #759 (split some commits to it)

Refactor Reline::Unicode vi_ ed_ em_ methods

Rewrite them with -250 lines.
Update questionable test case.

Performance

# before (slow because time complexity was O(n^2))
irb(main):002> Reline::Unicode.em_backward_word('a'*2000,2000)
processing time: 0.265469s
=> [2000, 2000]

# after
irb(main):002> Reline::Unicode.em_backward_word('a'*2000,2000) 
processing time: 0.001357s
=> 2000

@tompng tompng force-pushed the unicode_edit_func_refactor branch 2 times, most recently from 789d090 to 85f4405 Compare June 10, 2024 13:59
lib/reline/unicode.rb Outdated Show resolved Hide resolved
end
[byte_size, width, new_str]
gcs = line.byteslice(byte_pointer..).grapheme_clusters
nonwords = gcs.take_while { |c| c.encode(Encoding::UTF_8).match?(/\P{Word}/) }
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to encode gcs all at once before looping through them?
Also, all tests still pass even if I removed these encoding calls. Can we add a few cases for them?

Copy link
Member Author

Choose a reason for hiding this comment

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

encode will change bytesize, so I have to fix all encode(Encoding::UTF_8).grapheme_clusters in this file to return the correct bytesize in original encoding. I'll also add a test.

@tompng tompng marked this pull request as draft June 17, 2024 16:13
@tompng tompng marked this pull request as ready for review June 19, 2024 16:08
@tompng tompng marked this pull request as draft October 9, 2024 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants