Skip to content

Add an auto-generated unicode character category file#4605

Open
TheBlueMatt wants to merge 1 commit intolightningdevkit:mainfrom
TheBlueMatt:2026-05-unicode-autogen
Open

Add an auto-generated unicode character category file#4605
TheBlueMatt wants to merge 1 commit intolightningdevkit:mainfrom
TheBlueMatt:2026-05-unicode-autogen

Conversation

@TheBlueMatt
Copy link
Copy Markdown
Collaborator

1a01b5a added detection of unicode format characters in PrintableString, but used a hard-coded table which may eventually become out of date.

Here we switch to an auto-generated table, include all General_Category Other characters, and also ban unallocated code points.

Finally, CI validates that the file is kept up to date.

Written by Claude

@ldk-reviews-bot
Copy link
Copy Markdown

ldk-reviews-bot commented May 7, 2026

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@TheBlueMatt TheBlueMatt requested a review from tnull May 7, 2026 18:47
1a01b5a added detection of unicode
format characters in `PrintableString`, but used a hard-coded table
which may eventually become out of date.

Here we switch to an auto-generated table, include all
`General_Category` `Other` characters, and also ban unallocated
code points.

Finally, CI validates that the file is kept up to date.

Written by Claude
@TheBlueMatt TheBlueMatt force-pushed the 2026-05-unicode-autogen branch from b6f8c03 to bd75483 Compare May 7, 2026 18:49
Comment on lines +36 to +38
let is_other = is_unicode_general_category_other(c);
let is_unassigned = is_unicode_general_category_unassigned(c);
let c = if c.is_control() || is_other || is_unassigned {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: c.is_control() is now fully redundant — it checks Cc (Control), which is already covered by is_unicode_general_category_other (see 0x0000..=0x001F and 0x007F..=0x009F in unicode.rs). The old code needed it because is_format_char only covered Cf, but the new function covers all of Cc / Cf / Cs / Co.

Not a bug (the || short-circuits harmlessly), but it's potentially confusing because it suggests is_other doesn't handle control characters.

Suggested change
let is_other = is_unicode_general_category_other(c);
let is_unassigned = is_unicode_general_category_unassigned(c);
let c = if c.is_control() || is_other || is_unassigned {
let c = if is_unicode_general_category_other(c) || is_unicode_general_category_unassigned(c) {

@ldk-claude-review-bot
Copy link
Copy Markdown
Collaborator

ldk-claude-review-bot commented May 7, 2026

I've thoroughly reviewed every file and hunk in this diff. The generator logic, generated tables, CI integration, and module wiring all check out. Beyond my prior comment about the c.is_control() redundancy (which is the only real issue), I found no additional bugs, security problems, or logic errors in this review pass.

Review Summary

No new issues found.

Previously flagged (still applicable):

  • lightning-types/src/string.rs:38c.is_control() is redundant with is_unicode_general_category_other, which already covers all Cc (Control) characters (0x0000..=0x001F and 0x007F..=0x009F). Not a bug, but potentially misleading.

Verification notes:

  • The Python generator correctly handles First/Last range markers, validates category consistency across ranges, and treats absent codepoints as Cn per the Unicode spec.
  • The is_unicode_general_category_other and is_unicode_general_category_unassigned tables are disjoint and together cover all non-printable Unicode categories (Cc, Cf, Cs, Co, Cn).
  • Noncharacter codepoints (U+FDD0-U+FDEF, U+xFFFE-U+xFFFF) are correctly covered by the unassigned table.
  • The existing tests in string.rs cover the key cases (bidi overrides, Egyptian hieroglyph boundary at U+1343F/U+13440, control characters).

@codecov
Copy link
Copy Markdown

codecov Bot commented May 7, 2026

Codecov Report

❌ Patch coverage is 92.23301% with 40 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.15%. Comparing base (1a01b5a) to head (bd75483).
⚠️ Report is 30 commits behind head on main.

Files with missing lines Patch % Lines
lightning-types/src/unicode.rs 92.18% 40 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4605      +/-   ##
==========================================
+ Coverage   86.09%   86.15%   +0.06%     
==========================================
  Files         157      158       +1     
  Lines      108828   109327     +499     
  Branches   108828   109327     +499     
==========================================
+ Hits        93694    94193     +499     
  Misses      12519    12519              
  Partials     2615     2615              
Flag Coverage Δ
tests 86.15% <92.23%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants