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

bug(core): Keyman Core has an off-by-one error for nul ... > context(n) #13304

Open
mcdurdin opened this issue Feb 21, 2025 · 0 comments · May be fixed by #13314
Open

bug(core): Keyman Core has an off-by-one error for nul ... > context(n) #13304

mcdurdin opened this issue Feb 21, 2025 · 0 comments · May be fixed by #13314
Assignees
@mcdurdin
Labels
bug core/ Keyman Core
Milestone
B18S2

Comments

@mcdurdin
Copy link
Member

mcdurdin commented Feb 21, 2025 •
edited
Loading

It looks like handling is correct for if() but not for nul (but this needs to be verified) (verified, if() is working correctly, see tests in #13311)

store(&version) '17.0'
store(&TARGETS) 'any'

begin Unicode > use(main)

store(cons) 'mn'
store(ifx) '1'

group(main) using keys

c nul any(cons) + 'a' > 'x' context(1) 'z'

any(cons) + 'a' > '1' context(1)
nul any(cons) + 'b' > '2' context(2)    c <-- failing in debugger, 18.0.190-beta (and in Windows)
'x' any(cons) + 'c' > '3' context(2)
if(ifx = '1') any(cons) + 'd' > '4' context(2)

See related issue on documentation which documented this anomalous behaviour as 'correct':
@mcdurdin mcdurdin changed the title Correct Keyman Core's handling of nul ... > context(n) bug(core): Keyman Core has an off-by-one error for nul ... > context(n) Feb 21, 2025
@keymanapp-test-bot keymanapp-test-bot bot added bug core/ Keyman Core labels Feb 21, 2025
@mcdurdin mcdurdin self-assigned this Feb 21, 2025
@mcdurdin mcdurdin moved this to Todo in Keyman Feb 21, 2025
@mcdurdin mcdurdin added this to the B18S2 milestone Feb 21, 2025
@mcdurdin mcdurdin mentioned this issue Feb 21, 2025
Open
mcdurdin added a commit that referenced this issue Feb 21, 2025
@mcdurdin
chore(core): add unit tests to validate nul with index and context
2b9b8eb 
Add 4 test keyboards to validate `nul` and `if` used in conjunction with
`index` and `context` and corresponding references in core unit tests.

Add a script to rebuild baseline keyboards using a copy of kmcomp.exe
16.0.138; this is setup and tested only on Windows (YMMV on WINE, etc).

Add the 4 additional baseline test keyboards to kmcmplib unit tests for
build consistency between kmcomp 16 and kmcmplib 18 (all pass).

Note: two of the new Core tests currently fail. This is expected, see
issue #13304.

Fixes: #13303
@mcdurdin mcdurdin mentioned this issue Feb 21, 2025
Draft
mcdurdin added a commit that referenced this issue Feb 21, 2025
@mcdurdin
fix(core): include nul in context offset calculations in kmx processor
59254ba 
Two separate bugs addressed, with `index()` references and with
`context()` references -- both have the same root cause, of not taking
`nul` at the start of the context into account (as `nul` is not included
in the `m_miniContext` member, being a non-character). We already fixed
this issue for `if()` quite a long time ago, and some of the same
patterns can be with `m_miniContextIfLen` for example.

Fixes: #13304
@mcdurdin mcdurdin linked a pull request Feb 21, 2025 that will close this issue
Draft
@mcdurdin mcdurdin linked a pull request Feb 21, 2025 that will close this issue
fix(core): include nul in context offset calculations in kmx processor #13314
Draft
mcdurdin added a commit that referenced this issue Feb 21, 2025
@mcdurdin
fix(core): include nul in context offset calculations in kmx processor
b973bcc 
Two separate bugs addressed, with `index()` references and with
`context()` references -- both have the same root cause, of not taking
`nul` at the start of the context into account (as `nul` is not included
in the `m_miniContext` member, being a non-character). We already fixed
this issue for `if()` quite a long time ago, and some of the same
patterns can be with `m_miniContextIfLen` for example.

Fixes: #13304
Fixes: #13316
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mcdurdin mcdurdin

Labels
bug core/ Keyman Core
Projects
Keyman
Status: Todo
Milestone
B18S2
Development

Successfully merging a pull request may close this issue.

fix(core): include nul in context offset calculations in kmx processor keymanapp/keyman
1 participant
@mcdurdin