Closed Manishearth closed 1 year ago
Manishearth
commented
1 year ago r? @eggrobin or @sffc
Note for reviewers: Should be reviewed commit-by-commit. If you review the first two commits, you need not review #91, just let me know and I can merge that.
Looking for spec review from @eggrobin, Rust code review from @sffc , and ideally "is this the best way to implement this" from both.
Manishearth
commented
1 year ago We also probably should fuzz this crate to catch panics, and maybe also fuzz it against the reference implementation once all tests pass.
Manishearth
commented
1 year ago @sffc I'm hoping @eggrobin can do spec review, and you can do general Rusty code review as well as "is this the best way to implement this step" review.
FWIW If you review everything here including the first two commits I don't need to get review on https://github.com/servo/unicode-bidi/pull/91.
(expanded my r? comment to say so)
Manishearth
commented
1 year ago And all tests pass, now. Bonus fix for https://github.com/servo/unicode-bidi/issues/8
Manishearth
commented
1 year ago kick CI
Built on top of https://github.com/servo/unicode-bidi/pull/91. PR starts at "Add all fixups for retaining explicits".
Fixes #89 by doing the implementation hacks specified in section 5.2 Retaining BNs and Explicit Formatting Characters.
This fixes all of the character tests! (fixes #90).
We still have to fix some of the "basic" tests, which are unfortunately not as nicely labeled as the character tests.This also fixes the basic tests (fixes https://github.com/servo/unicode-bidi/issues/8 )This got mixed in with a bunch of other fixes since a lot of the code is nearby, a bunch of them fix things from N0. The main Big Fix in here besides the one around retaining explicit formatting is that the code now always handles lookahead/lookbehind by iterating within the run sequence only.
All in all, this PR contains: