test(developer): check correct use of u16chr when second parameter could be null #11894

markcsinclair · 2024-07-01T10:15:09Z

Fix the use of u16chr() with *q==0 in u16tok() bug (and anywhere else it is similarly misused) - see issue #11814

Tests for u16chr() and u16tok() (both variants) are included in gtest-kmxu16-test.cpp

Partially fixes: #11814 (another PR is needed for the first bug)

Related-to: #11910

@keymanapp-test-bot skip

keymanapp-test-bot · 2024-07-01T10:15:21Z

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

Developer
Keyboards
- Test Keyboards

Relates-to: #11814 See-also: #11894

mcdurdin · 2024-07-02T11:48:17Z

I independently encountered the u16tok bug. I have pushed my branch so you can compare: #11910.

Merge branch 'fix/developer/11814-kmx_u16-buffer-overrun' into fix/developer/correct-use-of-u16chr-when-second-parameter-could-be-null

…e-of-u16chr-when-second-parameter-could-be-null # Keyman Conventional Commit suggestions: # # - Link to a Sentry issue with git trailer: # Fixes: _MODULE_-_ID_ # - Give credit to co-authors: # Co-authored-by: _Name_ <_email_> # - Use imperative, present tense ('attach' not 'attaches', 'attached' etc) # - Don't include a period at the end of the title # - Always include a blank line before trailers # - More: https://github.com/keymanapp/keyman/wiki/Pull-Request-and-Commit-workflow-notes

…g of context before test cases

…e-of-u16chr-when-second-parameter-could-be-null # Keyman Conventional Commit suggestions: # # - Link to a Sentry issue with git trailer: # Fixes: _MODULE_-_ID_ # - Give credit to co-authors: # Co-authored-by: _Name_ <_email_> # - Use imperative, present tense ('attach' not 'attaches', 'attached' etc) # - Don't include a period at the end of the title # - Always include a blank line before trailers # - More: https://github.com/keymanapp/keyman/wiki/Pull-Request-and-Commit-workflow-notes

… and remove unncessary prefix from two cases

…e-of-u16chr-when-second-parameter-could-be-null # Keyman Conventional Commit suggestions: # # - Link to a Sentry issue with git trailer: # Fixes: _MODULE_-_ID_ # - Give credit to co-authors: # Co-authored-by: _Name_ <_email_> # - Use imperative, present tense ('attach' not 'attaches', 'attached' etc) # - Don't include a period at the end of the title # - Always include a blank line before trailers # - More: https://github.com/keymanapp/keyman/wiki/Pull-Request-and-Commit-workflow-notes

…nType() test

markcsinclair · 2024-07-22T10:44:23Z

Ready for review, although waiting for PR #11910 to be merged into master first.

…e-of-u16chr-when-second-parameter-could-be-null # Keyman Conventional Commit suggestions: # # - Link to a Sentry issue with git trailer: # Fixes: _MODULE_-_ID_ # - Give credit to co-authors: # Co-authored-by: _Name_ <_email_> # - Use imperative, present tense ('attach' not 'attaches', 'attached' etc) # - Don't include a period at the end of the title # - Always include a blank line before trailers # - More: https://github.com/keymanapp/keyman/wiki/Pull-Request-and-Commit-workflow-notes

markcsinclair · 2024-07-23T10:43:58Z

PR #11910 has been merged into master, and in turn into this PR, so it is ready to be reviewed.

Compared to PR #11910, this PR offers more extensive testing of u16chr() and the two variants of u16tok(), plus testing of two Compiler.cpp functions, ProcessEthnologueStore() and LineTokenType() which were (incorrectly) suspected of containing misuse of u16chr(). The testing on ProcessEthnologueStore() did identify a different bug (see #11955) however.

…e-of-u16chr-when-second-parameter-could-be-null

…e-of-u16chr-when-second-parameter-could-be-null # Keyman Conventional Commit suggestions: # # - Link to a Sentry issue with git trailer: # Fixes: _MODULE_-_ID_ # - Give credit to co-authors: # Co-authored-by: _Name_ <_email_> # - Use imperative, present tense ('attach' not 'attaches', 'attached' etc) # - Don't include a period at the end of the title # - Always include a blank line before trailers # - More: https://github.com/keymanapp/keyman/wiki/Pull-Request-and-Commit-workflow-notes

mcdurdin

Looking good, two small items -- one of which should probably be treated as a latent (if not currently visible) bug in the compiler?

Noting that the title and description suggest changes to the compiler code, not just the tests -- is there something missing?

mcdurdin · 2024-07-29T02:16:16Z

developer/src/kmcmplib/tests/gtest-compiler-test.cpp

+    // comment without following space
+    u16cpy(str, u"c");
+    p = str;
+    EXPECT_EQ(T_UNKNOWN, LineTokenType(&p));
+    EXPECT_EQ(0, p - str);


Hmm, I would have thought that this should be treated as a comment. It turns out that ReadLine() strips the comment before it ever gets to LineTokenType(), so we don't get failure:

keyman/developer/src/kmcmplib/src/Compiler.cpp

Lines 3404 to 3410 in bcec446

case L'c':

case L'C':

if ((p == str || iswspace(*(p - 1))) && iswspace(*(p + 1))) {

InComment = TRUE;

*p = L' ';

}

continue;

(In ReadLine() the input has a trailing \n so iswspace(*(p+1)) is true.)

I have left as is, except to add a comment to the test cf. ReadLine(). Should I open an issue?

Yeah, probably open an issue, add the low priority label to it.

developer/src/kmcmplib/tests/gtest-kmx_u16-test.cpp

markcsinclair · 2024-07-30T10:15:15Z

Noting that the title and description suggest changes to the compiler code, not just the tests -- is there something missing?

This PR originally contained a fix to the compiler whilst in draft, but then you developed a fix in PR #11910. I merged that into this PR, adopting your fix and extending my tests (see my comments above). I will change the name of this PR to a reflect its final test-only content.

…e-of-u16chr-when-second-parameter-could-be-null # Keyman Conventional Commit suggestions: # # - Link to a Sentry issue with git trailer: # Fixes: _MODULE_-_ID_ # - Give credit to co-authors: # Co-authored-by: _Name_ <_email_> # - Use imperative, present tense ('attach' not 'attaches', 'attached' etc) # - Don't include a period at the end of the title # - Always include a blank line before trailers # - More: https://github.com/keymanapp/keyman/wiki/Pull-Request-and-Commit-workflow-notes

…k_str_compare_to_strtok

…or 'c' with no following space

mcdurdin · 2024-07-30T10:56:27Z

This PR originally contained a fix to the compiler whilst in draft, but then you developed a fix in PR #11910. I merged that into this PR, adopting your fix and extending my tests (see my comments above). I will change the name of this PR to a reflect its final test-only content.

And ... I knew that and forgot. Sorry for the hassle!

mcdurdin

LGTM

keyman-server · 2024-07-31T18:04:06Z

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

fix(developer): add gtest-kmxu16-test.cpp for testing of kmx_u16.cpp

998e0ea

markcsinclair requested review from mcdurdin and darcywong00 as code owners July 1, 2024 10:15

markcsinclair marked this pull request as draft July 1, 2024 10:15

markcsinclair self-assigned this Jul 1, 2024

github-actions bot added developer/ developer/compilers/ fix labels Jul 1, 2024

keymanapp-test-bot bot added this to the A18S5 milestone Jul 1, 2024

markcsinclair added 3 commits July 1, 2024 11:31

fix(developer): add gtest-kmxu16-test.cpp to meson.build

47d853e

fix(developer): add three test cases for u16chr()

dbdd2ef

fix(developer): add two test cases for u16tok_delim test

093041f

mcdurdin added a commit that referenced this pull request Jul 2, 2024

fix(developer): prevent buffer overrun in u16tok

69d8b30

Relates-to: #11814 See-also: #11894

mcdurdin mentioned this pull request Jul 2, 2024

fix(developer): prevent buffer overrun in u16tok #11910

Merged

markcsinclair and others added 4 commits July 4, 2024 10:51

fix(developer): change file/class nomenclature to match #11910

34859c3

chore(developer): merge #11910 into #11894

7efd46f

Merge branch 'fix/developer/11814-kmx_u16-buffer-overrun' into fix/developer/correct-use-of-u16chr-when-second-parameter-could-be-null

fix(developer): combine tests from #11910 and this PR

82d041a

chore(developer): add more delimiter tests for u16tok

bf5b5af

mcdurdin marked this pull request as ready for review July 5, 2024 08:10

mcdurdin marked this pull request as draft July 5, 2024 08:11

darcywong00 modified the milestones: A18S5, A18S6 Jul 5, 2024

markcsinclair added 6 commits July 8, 2024 10:01

fix(developer): add u16chr_compare_to_strchr test

629ae86

fix(developer): remove duplicate assertion

6a1ffb7

fix(developer): edits for consistency in use of str buffer and settin…

a486dd7

…g of context before test cases

fix(developer): edits for consistency in u16cmp assertions

24c67df

fix(developer): additional assertions on ctx

a808833

markcsinclair added 4 commits July 18, 2024 11:06

fix(developer): add LineTokenType test and initial three test cases

3ea80a8

fix(developer): add three more test cases to LineTokenType() test

caa3de1

fix(developer): add four additional test cases to LineTokenType test,…

66b6fcf

… and remove unncessary prefix from two cases

darcywong00 modified the milestones: A18S6, A18S7 Jul 19, 2024

markcsinclair added 2 commits July 22, 2024 10:59

fix(developer): add five additional whitespace test cases to LineToke…

46842f5

…nType() test

markcsinclair mentioned this pull request Jul 22, 2024

bug(developer): incorrect handling of second parameter of index in kmcmplib compiler #11814

Closed

8 tasks

markcsinclair marked this pull request as ready for review July 23, 2024 10:44

markcsinclair added 2 commits July 23, 2024 20:52

chore(developer): Merge branch 'master' into fix/developer/correct-us…

42210a0

…e-of-u16chr-when-second-parameter-could-be-null

mcdurdin requested changes Jul 29, 2024

View reviewed changes

markcsinclair changed the title ~~fix(developer): correct use of u16chr when second parameter could be null~~ test(developer): check correct use of u16chr when second parameter could be null Jul 30, 2024

github-actions bot added the test Any acceptance test issue label Jul 30, 2024

chore(developer): correct copy-paste error in finl test case of u16to…

298853b

…k_str_compare_to_strtok

markcsinclair removed the fix label Jul 30, 2024

chore(developer): add additional comment to LineTokenType test case f…

a53ea2c

…or 'c' with no following space

markcsinclair requested a review from mcdurdin July 30, 2024 10:52

mcdurdin approved these changes Jul 30, 2024

View reviewed changes

markcsinclair merged commit e8e28b6 into master Jul 31, 2024
4 checks passed

markcsinclair deleted the fix/developer/correct-use-of-u16chr-when-second-parameter-could-be-null branch July 31, 2024 10:25

markcsinclair mentioned this pull request Jul 31, 2024

bug(developer): incorrect handling of whitespace in comments for kmcmplib ReadLine() and LineTokenType() #12070

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(developer): check correct use of u16chr when second parameter could be null #11894

test(developer): check correct use of u16chr when second parameter could be null #11894

markcsinclair commented Jul 1, 2024 •

edited

Loading

keymanapp-test-bot bot commented Jul 1, 2024 •

edited

Loading

mcdurdin commented Jul 2, 2024

markcsinclair commented Jul 22, 2024

markcsinclair commented Jul 23, 2024

mcdurdin left a comment

mcdurdin Jul 29, 2024

markcsinclair Jul 30, 2024

mcdurdin Jul 30, 2024

markcsinclair commented Jul 30, 2024

mcdurdin commented Jul 30, 2024

mcdurdin left a comment

keyman-server commented Jul 31, 2024

	case L'c':
	case L'C':
	if ((p == str \|\| iswspace((p - 1))) && iswspace((p + 1))) {
	InComment = TRUE;
	*p = L' ';
	}
	continue;

test(developer): check correct use of u16chr when second parameter could be null #11894

test(developer): check correct use of u16chr when second parameter could be null #11894

Conversation

markcsinclair commented Jul 1, 2024 • edited Loading

keymanapp-test-bot bot commented Jul 1, 2024 • edited Loading

User Test Results

Test Artifacts

mcdurdin commented Jul 2, 2024

markcsinclair commented Jul 22, 2024

markcsinclair commented Jul 23, 2024

mcdurdin left a comment

Choose a reason for hiding this comment

mcdurdin Jul 29, 2024

Choose a reason for hiding this comment

markcsinclair Jul 30, 2024

Choose a reason for hiding this comment

mcdurdin Jul 30, 2024

Choose a reason for hiding this comment

markcsinclair commented Jul 30, 2024

mcdurdin commented Jul 30, 2024

mcdurdin left a comment

Choose a reason for hiding this comment

keyman-server commented Jul 31, 2024

markcsinclair commented Jul 1, 2024 •

edited

Loading

keymanapp-test-bot bot commented Jul 1, 2024 •

edited

Loading