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.

Sign up for GitHub

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

Jump to bottom

feat(core) actual regex 🙀 #9440

Merged
merged 18 commits into from
Aug 16, 2023
Merged

feat(core) actual regex 🙀 #9440

merged 18 commits into from
Aug 16, 2023

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Aug 8, 2023 •
edited
Loading

  • test for transform cases that require regex
  • enable icui18n
  • hook up icu regex engine
  • try not to break existing ICU use

for #9121

Note that #9467 will split the ICU stuff out in wasm.

@keymanapp-test-bot skip

@srl295 srl295 added this to the A17S19 milestone Aug 8, 2023
@srl295 srl295 self-assigned this Aug 8, 2023
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Aug 8, 2023 •
edited
Loading

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

@srl295
feat(core): regex test 🙀
1472bc5 
For: #9121
@srl295
feat(core): update icu dependencies 🙀
5c38b18 
- enable icui18n
- .. without breaking anything else in ldml tests

For: #9121
@srl295 srl295 force-pushed the feat/core/9121-regex-epic-ldml branch from cbd1e6c to 5c38b18 Compare August 8, 2023 23:45
@srl295
Copy link
Member Author

srl295 commented Aug 8, 2023

just a weather balloon 🎈 to see how the build fares…

@srl295
Copy link
Member Author

srl295 commented Aug 9, 2023

build succeeds! 🎉

@srl295
feat(core): u32string_to_u16string 🙀
742373b 
For: #9121
@srl295
feat(core): regex 🙀
022265e 
- update more test deps to link with ICU…

For: #9121
@srl295
feat(core): regex match-side 🙀
1aba900 
- match side is working
- lots of string 'schlepping!

For: #9121
@srl295
feat(core): regex apply-side 🙀
aecb7c5 
- apply side is working using replaceFirst()
- mapTo is a not-yet, that will be a special case

For: #9121
@srl295
Copy link
Member Author

srl295 commented Aug 10, 2023

ICU unresolved external symbols in the Windows 32 build. I'll evaluate when I get back to my Windows box.

@srl295
Merge branch 'feat/core/9119-marker-core-epic-ldml' into feat/core/91…
4fc15c9 
…21-regex-epic-ldml
@srl295
chore(core): regex build 🙀
36b4022 
- fix linkage for some scenarios

For: #9121
@srl295
chore(core): regex cleanup 🙀
75fcc5f 
- cache the icu::RegexPattern
- rename USet to avoid a conflict with the real one!

For: #9121
@srl295
Copy link
Member Author

srl295 commented Aug 10, 2023 •
edited
Loading

looks like keyman32.vcxproj probably needs an explicit linkage to the ICU libraries? I thought they were static here, hmm.

Base automatically changed from feat/core/9119-marker-core-epic-ldml to master August 11, 2023 00:06
@srl295
Copy link
Member Author

srl295 commented Aug 11, 2023

@mcdurdin @rc-swag ready for some initial eyes.. not ready to merge

mcdurdin
mcdurdin reviewed Aug 11, 2023
View reviewed changes
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

looking "good" so far, conceptually. The ICU libraries are static-linked, right (essential on Windows)?

core/src/ldml/ldml_transforms.cpp Outdated Show resolved Hide resolved
core/src/ldml/ldml_transforms.cpp Outdated Show resolved Hide resolved
core/src/ldml/ldml_transforms.cpp Outdated Show resolved Hide resolved
core/src/meson.build Outdated Show resolved Hide resolved
@srl295
Copy link
Member Author

srl295 commented Aug 11, 2023

Thanks… That's the goal, I will have to check to see if it hit the mark or not

@srl295
Copy link
Member Author

srl295 commented Aug 11, 2023

ah, i had forgotten a push before asking for review… 

srl295 and others added 3 commits August 11, 2023 15:29
@srl295 @mcdurdin
Apply suggestions from code review
548acaa 
Co-authored-by: Marc Durdin <marc@durdin.net>
@srl295
chore(core): regex cleanup 🙀
ad03a5c 
- more comments

For: #9121
@srl295
feat(core): regex cleanup 🙀
7a31252 
- restructure to not allocate the Matcher twice, save a little processing

For: #9121
@srl295 srl295 marked this pull request as ready for review August 11, 2023 21:48
@srl295 srl295 requested a review from rc-swag as a code owner August 11, 2023 21:48
@srl295 srl295 requested a review from jahorton August 11, 2023 21:50
@srl295
Copy link
Member Author

srl295 commented Aug 11, 2023 •
edited
Loading

Okay. I'm going to call this one ready to review as far as the implementation goes. Then i'm working on a sub-PR focused on the build size.

Won't merge this until we have some good buy-in on the build size.

@srl295
Copy link
Member Author

srl295 commented Aug 15, 2023

okay, given #9467 I think the path for this one should be:

  • pass tests
  • merge it

It will make wasm bigger for now, but that won't affect KMW yet. But if 9467 is a higher priority before merge, that can be done too. Without KMW calling into wasm, i don't have a way to test the JS callback unless I make a fake test harness.

@mcdurdin
Copy link
Member

okay, given #9467 I think the path for this one should be:

  • pass tests
  • merge it

SGTM.

mcdurdin
mcdurdin approved these changes Aug 16, 2023
View reviewed changes
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

LGTM

@srl295 srl295 merged commit 233c0d7 into master Aug 16, 2023
2 checks passed
@srl295 srl295 deleted the feat/core/9121-regex-epic-ldml branch August 16, 2023 12:12
@keyman-server
Copy link
Collaborator

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

@srl295 srl295 mentioned this pull request Sep 25, 2023
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcdurdin mcdurdin mcdurdin approved these changes

@rc-swag rc-swag Awaiting requested review from rc-swag rc-swag is a code owner

@jahorton jahorton Awaiting requested review from jahorton

@ermshiperete ermshiperete Awaiting requested review from ermshiperete ermshiperete is a code owner

Assignees

@srl295 srl295

Labels
core/ Keyman Core epic-ldml feat
Projects
None yet
Milestone
A17S19
Development

Successfully merging this pull request may close these issues.
3 participants
@srl295 @mcdurdin @keyman-server