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(core): cross-segment markers 🙀 #10394

Merged
merged 9 commits into from
Jan 19, 2024

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Jan 16, 2024

  • use a normalized deque of (char32_t, marker_num) pairs instead of a map
  • use the NFD normalizer to find boundaries for segmenting before processing markers
  • test improvements

#10369

@keymanapp-test-bot skip

@srl295 srl295 added this to the A17S30 milestone Jan 16, 2024
@srl295 srl295 self-assigned this Jan 16, 2024
@keymanapp-test-bot
Copy link

User Test Results

Test specification and instructions

User tests are not required

@github-actions github-actions bot added core/ Keyman Core feat labels Jan 16, 2024
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Jan 16, 2024

@srl295 srl295 force-pushed the feat/core/10369-marker-sequence-again-epic-ldml branch from 94dbff9 to 565a1ca Compare January 18, 2024 01:57
Base automatically changed from feat/developer/10319-remove-old-escape-epic-ldml to master January 18, 2024 16:38
@srl295 srl295 force-pushed the feat/core/10369-marker-sequence-again-epic-ldml branch from 565a1ca to 6f5cee0 Compare January 18, 2024 19:16
- split out parse_next_marker()
- use NFD safe boundaries to segment marker interaction

For: #10369
@srl295 srl295 force-pushed the feat/core/10369-marker-sequence-again-epic-ldml branch from 6f5cee0 to 3ccffe9 Compare January 18, 2024 19:52
- easier to understand loop
- comments

For: #10369
@srl295 srl295 changed the title feat(core): work on cross segment markers 🙀 feat(core): cross-segment markers 🙀 Jan 18, 2024
@srl295 srl295 marked this pull request as ready for review January 18, 2024 20:40
@srl295 srl295 marked this pull request as draft January 18, 2024 21:54
@srl295 srl295 linked an issue Jan 18, 2024 that may be closed by this pull request
@srl295 srl295 marked this pull request as ready for review January 19, 2024 00:15
core/tests/unit/ldml/test_transforms.cpp Outdated Show resolved Hide resolved
}
}
// assert(marki == map.rend()); // that we consumed everything
Copy link
Member

Choose a reason for hiding this comment

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

Commented code here?

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch Should have been map2, but it wasn't the right assert. added a better assert now.

core/src/ldml/ldml_markers.cpp Outdated Show resolved Hide resolved
@srl295 srl295 merged commit 5d62d72 into master Jan 19, 2024
17 checks passed
@srl295 srl295 deleted the feat/core/10369-marker-sequence-again-epic-ldml branch January 19, 2024 16:12
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 17.0.248-alpha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core/ Keyman Core feat
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(core): segmented marker normalization 🙀
3 participants