UTF-16 support #109

jfkthame · 2023-10-16T12:59:54Z

This aims to provide "native" UTF-16 support, with *U16 versions of InitialInfo, BidiInfo, and Paragraph structs that are parallel to the existing types but work on a [u16] text buffer instead of a str.

Fixes #108

src/lib.rs

Manishearth · 2023-10-16T15:51:19Z

This is pretty big and I'm unlikely to be able to review it soon, fwiw.

jfkthame · 2023-10-16T17:31:05Z

This is pretty big and I'm unlikely to be able to review it soon, fwiw.

Yeah - sorry, I know there's a lot of it!

To make it more approachable, I tried to post it as a series of logical steps that build on each other (and I think it builds and passes tests at all stages of the patch queue, IIRC).

Much of it is moving (rather than modifying) existing code, and then duplicating (with str -> [u16] changes) the structs that are the public API. So there's actually not much new code here; once the TextSource trait is available, the rest is pretty mechanical.

Manishearth

Review has reached e3bad1f, which seems rather big and might take a while to review. ~~If you can splti it into smaller commits that would be appreciated.~~. Already working through it now.

Please avoid force pushing changes to earlier commits since I've already reviewed them, github shows interdiffs but that's only useful once one has reviewed the entire PR. I'll let you know when it's okay to force-push again; in case you care about maintaining a clean git history.

src/lib.rs

src/implicit.rs

Manishearth · 2023-10-26T21:43:16Z

src/implicit.rs

+        let mut actual_index = level_run.start;
+
+        while actual_index < level_run.end {
+            let (ch, len) = text.char_at(actual_index).unwrap();


issue: no panics, please.

The older structure of the code was intentional to use char_indices, please retain something similar. I think we can add an indexing method that returns a &Self to TextSource and then call char_indices() on it

src/lib.rs

Manishearth

Overall looks good. Please fix anything marked issue/nit and we can merge this.

(Fine to force push again, now that I've reviewed the entire thing)

Thanks for doing this!

src/lib.rs

Manishearth · 2023-10-26T22:37:13Z

src/lib.rs

+    /// [Rule L4]: https://www.unicode.org/reports/tr9/#L4
+    #[cfg_attr(feature = "flame_it", flamer::flame)]
+    pub fn reorder_line(&self, para: &ParagraphInfo, line: Range<usize>) -> Cow<'text, [u16]> {
+        if !level::has_rtl(&self.levels[line.clone()]) {


suggestion: would be nice to also refactor this out, perhaps in a way that allows the caller to pass in the encode_utf16

I didn't do this for now, as it's a pretty short function and it wasn't really clear to me how to do it most tidily. But maybe as a followup...

Manishearth · 2023-10-26T22:39:03Z

src/lib.rs

@@ -1590,6 +1597,19 @@ mod tests {
            }
        );

+        let text = &to_utf16(text);
+        assert_eq!(


suggestion: would be nice to refactor this double-assert into a helper function

Manishearth · 2023-10-26T22:42:05Z

src/lib.rs

+///
+/// Contains the text paragraphs and `BidiClass` of its characters.
+#[derive(PartialEq, Debug)]
+pub struct InitialInfoU16<'text> {


thought: it would be ideal for us to have a GenericBidiInfo, GenericInitialInfo, and GenericParagraph types that abstract over an encoding, where the UTF8 one is just a type alias.

This might be more work. I'm fine with having separate UTF16 types for now as long as they're in a module.

Yes, that'd be nice; my grasp of Rust is still pretty weak, though, so I haven't attempted this as yet.

jfkthame · 2023-10-27T16:59:59Z

Huh, I wanted to completely remove pub from the TextSource trait, and locally that works fine for me, but the CI build here doesn't seem to like it. Sigh.

…xt being processed. The core bidi algorithms will be adapted to operate on TextSource, and we will provide implementations for both `str` and `[u16]` text buffers.

…-based helper. This is in preparation for creating a UTF-16 version of InitialInfo, which will rely on the same TextSource-based helper function.

In preparation for creating a UTF-16 version.

Because Paragraph has a BidiInfo field, and we're going to create a separate BidiInfoU16 to handle UTF-16 data, we'll need a separate ParagraphU16 as well.

…ormance.

Manishearth · 2023-10-28T21:31:02Z

I'm comfortable increasing to 1.56, but yes if we have an alternative we should use it

I didn't even realize we depended on syn, I would love for this to be a leaf crate

mbrubeck · 2023-10-28T21:53:40Z

Currently we depend on syn only with the optional serde or flamer features enabled. By default this crate has zero dependencies.

Manishearth · 2023-10-28T22:07:24Z

ah, yeah, I'm fine with breaking msrv for those features only

we can tweak CI to not run those

jfkthame · 2023-10-29T10:24:33Z

I took @mbrubeck's suggestion (thanks!) to implement the sealed-trait pattern manually, which seems straightforward, so that removes the dependency on the sealed crate.

Unfortunately, 1.36.0 still fails, because REPLACEMENT_CHARACTER, from_u32(), and decode_utf16() are all in the std version of char, not provided by the primitive char type.

@Manishearth Should we just provide local versions of these, too? They seem like they should be simple enough...

mbrubeck · 2023-10-29T20:58:32Z

Unfortunately, 1.36.0 still fails, because REPLACEMENT_CHARACTER, from_u32(), and decode_utf16() are all in the std version of char, not provided by the primitive char type.

You can use these as core::char::REPLACEMENT_CHARACTER, core::char::from_u32, etc.

jfkthame · 2023-10-29T23:01:32Z

Unfortunately, 1.36.0 still fails, because REPLACEMENT_CHARACTER, from_u32(), and decode_utf16() are all in the std version of char, not provided by the primitive char type.

You can use these as core::char::REPLACEMENT_CHARACTER, core::char::from_u32, etc.

Huh, you're right -- the error message told me I needed std, but core also works. So it's sufficient to just say use core::char; at the beginning of the module to resolve this.

Next up is that I put a lot of the tests and expected-results into arrays, and then use for t in tests { ... } to run the same calls & assertions over a whole set of tests. That works fine for me with current Rust, but again, 1.36.0 doesn't like it, so looks like that'll need to be reworked. I'll try to have a look at it, but any hints/suggestions are welcome!

mbrubeck · 2023-10-29T23:34:56Z

There are various options, but one simple one is to change the arrays to vecs: let tests = vec![ /* ... */ ];

It looks like Rust 1.36 also chokes on one use of format! to create an &str in the test module. This can probably be replaced by a string literal with Unicode character escapes, or you can move the format! to a binding outside of the tests vector like:

        let fsi_rtl_pdi_ltr = format!("{}א{}a", chars::FSI, chars::PDI);
        let tests = vec![
            // ...
            (
                &fsi_rtl_pdi_ltr,
                // ...

…CI with Rust 1.36.0

jfkthame · 2023-10-30T13:38:23Z

Thanks for all the feedback & suggestions -- I don't know my way around the Rust ecosystem much yet, so it takes me a while to find things!
At this point I think this should be ready to merge, AFAICS.

Manishearth · 2023-10-30T17:38:42Z

Apologies for the repeated force-push spam! It was all building fine for me locally, but took some repeated attempts to get the CI jobs happy. Not sure about the failing 1.36.0 job? Please let me know what I ought to do, if anything, or can we ignore it? Thanks!

When dealing with a PR this big it's better to just make fixup commits and clean up the commit history later (or at least explain the changes between force pushes in a comment)

Manishearth · 2023-10-30T17:41:42Z

Hmm, I went through all of the new interdiffs and could not see any of the issues being addressed. I might have missed an interdiff or a commit, I might have to go through this commit by commit again.

(This is why I was discouraging force pushes)

Manishearth

Almost there, still a couple of my comments that haven't been completely addressed.

I didn't redo the commit by commit review but I went through my old comments and looked at what was fixed.

For future fixes please only make new commits. You can use git commit --fixup <commit hash> to stage a commit for being squashed into a previous commit later; we can clean up the commit history before merging.

Though the changes I have requested probably are fine in their own commits.

Manishearth · 2023-10-30T17:47:12Z

src/implicit.rs

+        let mut actual_index = level_run.start;
+
+        for ch in text.subrange(level_run.clone()).chars() {
+            let char_len = T::char_len(ch);


nit: let's use char_indices here instead of recomputing char_len. there's a chance the optimizer will figure this out, I'd rather not take that chance.

Again, the old code had the right structure: run char_indices and add to level_run.start.

Done in 7866637

Manishearth · 2023-10-30T17:48:38Z

src/lib.rs

+/// If doing so, you may prefer to use [`reordered_levels_per_char()`] instead
+/// to avoid non-character indices.
+///
+/// For an all-in-one reordering solution, consider using [`Self::reorder_visual()`].


nit: this line and the one above it is not relevant for a private API.

This should just be a low level explanation of the function: what it does (the first line of docs) and what spec stuff it covers (the mention of rule L1)

Done in 69aba46

src/lib.rs

…utf16 submodule public.

…tions.

Manishearth · 2023-10-30T19:39:46Z

src/implicit.rs

@@ -256,6 +256,7 @@ pub fn resolve_neutral<'a, D: BidiDataSource, T: TextSource<'a> + ?Sized>(
    original_classes: &[BidiClass],
    processing_classes: &mut [BidiClass],
 ) where
+    <T as TextSource<'a>>::CharIndexIter: Iterator<Item = (usize, char)>,


nit: all of these bounds should live on the TextSource trait, not on all these where clauses

…ke the utf16 submodule public.

Manishearth

looks good with one small change. feel free to force push / squash and I'll merge

Manishearth · 2023-10-30T20:31:55Z

src/lib.rs

+/// This trait is sealed and cannot be implemented for types outside this crate.
+pub trait TextSource<'text>: private::Sealed
+where
+    <Self as TextSource<'text>>::CharIndexIter: Iterator<Item = (usize, char)>,


not: not in the where bounds, they can go on the associated types directly: type CharIndexIterator: Iterator<...>

Manishearth · 2023-10-30T23:46:59Z

Thanks!

jfkthame force-pushed the utf-16 branch from 87ccd00 to 8d372d6 Compare October 16, 2023 13:53

Manishearth reviewed Oct 16, 2023

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

Manishearth requested changes Oct 16, 2023

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

jfkthame requested a review from Manishearth October 16, 2023 17:58

jfkthame force-pushed the utf-16 branch 2 times, most recently from 3907bc8 to 94ba2ab Compare October 23, 2023 13:56

Manishearth reviewed Oct 26, 2023

View reviewed changes

jfkthame force-pushed the utf-16 branch 2 times, most recently from 548ab42 to f20df42 Compare October 27, 2023 16:55

jfkthame force-pushed the utf-16 branch from f20df42 to 46f741f Compare October 27, 2023 17:00

jfkthame added 8 commits October 27, 2023 18:06

[utf-16] Create a TextSource trait to be the abstract interface to te…

e30d741

…xt being processed. The core bidi algorithms will be adapted to operate on TextSource, and we will provide implementations for both `str` and `[u16]` text buffers.

[utf-16] Adapt explicit.rs to use the TextSource trait.

a9eb4ae

[utf-16] Adapt implicit.rs to use the TextSource trait.

f9affd5

[utf-16] Factor out the processing in InitialInfo to use a TextSource…

5ba5638

…-based helper. This is in preparation for creating a UTF-16 version of InitialInfo, which will rely on the same TextSource-based helper function.

[utf-16] Refactor BidiInfo to move algorithms into helper functions.

29cae27

In preparation for creating a UTF-16 version.

[utf-16] Factor out work from Paragraph into a helper function.

666b5ce

Because Paragraph has a BidiInfo field, and we're going to create a separate BidiInfoU16 to handle UTF-16 data, we'll need a separate ParagraphU16 as well.

[utf-16] Create a UTF-16 version of InitialInfo.

229ec5a

[utf-16] Create a UTF-16 version of BidiInfo.

3a3b1d0

jfkthame force-pushed the utf-16 branch 2 times, most recently from 2679f95 to bdc0822 Compare October 27, 2023 17:20

jfkthame added 4 commits October 27, 2023 18:22

[utf-16] Create a UTF-16 version of Paragraph.

1f53093

[utf-16] Duplicate testcases to exercise the UTF-16 APIs.

3656603

[utf-16] Check the BidiInfoU16 vs BidiInfo results in test_basic_conf…

f04d399

…ormance.

[utf-16] Add UTF-16 versions of benches.

0bc8c03

jfkthame force-pushed the utf-16 branch from bdc0822 to 0bc8c03 Compare October 27, 2023 17:22

[utf-16] Manually implement sealed-trait pattern for TextSource.

6fb622d

jfkthame force-pushed the utf-16 branch 2 times, most recently from c533080 to 604fdb0 Compare October 30, 2023 11:42

jfkthame added 2 commits October 30, 2023 11:44

[utf-16] Explicitly use core::char to resolve compilation failure in …

005e92e

…CI with Rust 1.36.0

[utf-16] Fix tests to be compatible with 1.36.0

c3d82dc

jfkthame force-pushed the utf-16 branch from 604fdb0 to c3d82dc Compare October 30, 2023 11:44

Manishearth requested changes Oct 30, 2023

View reviewed changes

jfkthame added 3 commits October 30, 2023 19:36

[utf-16] Don't expose ...U16 types in the base module, just make the …

74a631e

…utf16 submodule public.

[utf-16] Use char_indices() to iterate in identify_bracket_pairs().

7866637

fixup! [utf-16] Refactor BidiInfo to move algorithms into helper func…

69aba46

…tions.

Manishearth reviewed Oct 30, 2023

View reviewed changes

fixup! [utf-16] Don't expose ...U16 types in the base module, just ma…

52cc9ee

…ke the utf16 submodule public.

Manishearth approved these changes Oct 30, 2023

View reviewed changes

[utf-16] Move iterator bounds to the TextSource trait

ccda52f

jfkthame force-pushed the utf-16 branch from 512846a to ccda52f Compare October 30, 2023 22:56

Manishearth approved these changes Oct 30, 2023

View reviewed changes

Manishearth merged commit e699bf1 into servo:master Oct 30, 2023
6 checks passed

Manishearth mentioned this pull request Oct 30, 2023

Make types generic over encoding #113

Open

jfkthame deleted the utf-16 branch October 31, 2023 08:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UTF-16 support #109

UTF-16 support #109

jfkthame commented Oct 16, 2023

Manishearth commented Oct 16, 2023

jfkthame commented Oct 16, 2023

Manishearth left a comment •

edited

Loading

Manishearth Oct 26, 2023

Manishearth left a comment

Manishearth Oct 26, 2023

jfkthame Oct 27, 2023

Manishearth Oct 26, 2023

Manishearth Oct 26, 2023

jfkthame Oct 27, 2023

jfkthame commented Oct 27, 2023

Manishearth commented Oct 28, 2023

mbrubeck commented Oct 28, 2023

Manishearth commented Oct 28, 2023

jfkthame commented Oct 29, 2023

mbrubeck commented Oct 29, 2023

jfkthame commented Oct 29, 2023

mbrubeck commented Oct 29, 2023

jfkthame commented Oct 30, 2023

Manishearth commented Oct 30, 2023 •

edited

Loading

Manishearth commented Oct 30, 2023

Manishearth left a comment

Manishearth Oct 30, 2023

jfkthame Oct 30, 2023

Manishearth Oct 30, 2023

jfkthame Oct 30, 2023

Manishearth Oct 30, 2023

Manishearth left a comment

Manishearth Oct 30, 2023

Manishearth commented Oct 30, 2023

UTF-16 support #109

UTF-16 support #109

Conversation

jfkthame commented Oct 16, 2023

Manishearth commented Oct 16, 2023

jfkthame commented Oct 16, 2023

Manishearth left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfkthame commented Oct 27, 2023

Manishearth commented Oct 28, 2023

mbrubeck commented Oct 28, 2023

Manishearth commented Oct 28, 2023

jfkthame commented Oct 29, 2023

mbrubeck commented Oct 29, 2023

jfkthame commented Oct 29, 2023

mbrubeck commented Oct 29, 2023

jfkthame commented Oct 30, 2023

Manishearth commented Oct 30, 2023 • edited Loading

Manishearth commented Oct 30, 2023

Manishearth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth commented Oct 30, 2023

Manishearth left a comment •

edited

Loading

Manishearth commented Oct 30, 2023 •

edited

Loading