Runes, a bi-directional code-point iterator/iterable.

Bi-directional iterator for runes of a string.

Committed: https://code.google.com/p/dart/source/detail?r=18382

[Lasse Reichstein Nielsen, 2013-01-30 12:12:34]

What do you think?

[floitsch, 2013-01-30 13:12:13]

Adding Erik.
Like it, but we need to expose the rawIndex (and possibly the string it's
iterating over).

[erikcorry, 2013-01-30 13:26:55]

Needs a test (I don't think it works, seems to use the first surrogate as the
low bits, should be the high bits).

https://codereview.chromium.org/12094056/diff/1/sdk/lib/core/string.dart
File sdk/lib/core/string.dart (right):

https://codereview.chromium.org/12094056/diff/1/sdk/lib/core/string.dart#newc...
sdk/lib/core/string.dart:237: bool moveNext() {
This one is factored into two functions, whereas the exactly analogous
movePrevious is monolithic.  They should be the same.

[Lasse Reichstein Nielsen, 2013-01-30 14:03:37]

https://codereview.chromium.org/12094056/diff/1/sdk/lib/core/string.dart
File sdk/lib/core/string.dart (right):

https://codereview.chromium.org/12094056/diff/1/sdk/lib/core/string.dart#newc...
sdk/lib/core/string.dart:237: bool moveNext() {
ACK. I was using _readCharForward in the .at constructor too, but decided
against that.

https://codereview.chromium.org/12094056/diff/1/sdk/lib/core/string.dart#newc...
sdk/lib/core/string.dart:256: 0x10000 + ((nextCodeUnit & 0x3FF) << 10) |
(codeUnit & 0x3FF);
Good catch, these two are swapped (copy-n-paste bug from movePrevious).

[Lasse Reichstein Nielsen, 2013-02-12 14:05:45]

PTAL, ready to commit.

[floitsch, 2013-02-12 14:23:10]

LGTM.

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart
File sdk/lib/core/string.dart (right):

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:309: int get first {
You think it's worth specializing "first" ?

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:369: : string = string, _position = 0, _nextPosition =
0;
not necessary, but reads better with "this.string = string".

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:374: * A [moveNext] will make the following code point
the current value, and a
don't use "code point", but "rune".
Not perfect either, but maybe you can get something out of the following
proposed description:
A [moveNext] will make the rune that is positioned at the [index]th code unit
become the current value. A [movePrevious] will make the preceding rune, not
using the code unit at the [index]th position, the current value.

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:375: * [movePrevious] will make the preceding code
pount the current value.
point.

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:400: * If the current rune is null, this is the index
of the rune that
I would prefer if rawIndex was null or -1 then.

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:408: * Setting a negative [rawIndex], or one greater
than [:string.length:],
If we allow "string.length" we should maybe allow "-1" too (which would be
equivalent to "reset").

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:425: * Setting a negative [rawIndex], or one greater
than [:string.length:],
ditto. Maybe we should allow "-1".

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:443: * Returns zero if the current rune
unfinished sentence.

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:464: if (_nextPosition == string.length) {
_position. No need for _nextPosition anymore.

[Lasse Reichstein Nielsen, 2013-02-12 15:13:23]

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart
File sdk/lib/core/string.dart (right):

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:309: int get first {
Probably not. Let's drop it.

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:369: : string = string, _position = 0, _nextPosition =
0;
On 2013/02/12 14:23:10, floitsch wrote:
> not necessary, but reads better with "this.string = string".

Done.

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:374: * A [moveNext] will make the following code point
the current value, and a
Reworded.

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:375: * [movePrevious] will make the preceding code
pount the current value.
On 2013/02/12 14:23:10, floitsch wrote:
> point.

Done.

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:400: * If the current rune is null, this is the index
of the rune that
This way, someone can give you an uninitialized iterator, and you can check
where it will start, and restore it to that position later, without doing any
moves.

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:408: * Setting a negative [rawIndex], or one greater
than [:string.length:],
I'd disallow string.length then. 

Positions in strings (or any sequence) are not of the characters, but of the
fenceposts between characters.
That's why 0..length is allowed in arguments to String.substring.

In this case, because we expect to do a moveNext immediately, I'm all for
disallowing length.

And no, -1 is not the same as reset(0).

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:425: * Setting a negative [rawIndex], or one greater
than [:string.length:],
For what?
reset(0) is a reset to the beginning of the string.
reset(string.length) positions you just after the last rune.
It works here, like <deity> intended indices to work!

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:443: * Returns zero if the current rune
On 2013/02/12 14:23:10, floitsch wrote:
> unfinished sentence.

Done.

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:464: if (_nextPosition == string.length) {
On 2013/02/12 14:23:10, floitsch wrote:
> _position. No need for _nextPosition anymore.

Done.

[Lasse Reichstein Nielsen, 2013-02-12 15:20:23]

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart
File sdk/lib/core/string.dart (right):

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:400: * If the current rune is null, this is the index
of the rune that
I've changed it to return null anyway, if there is no current rune. It's an easy
change back if someone wants it.

[erikcorry, 2013-02-13 08:58:54]

https://codereview.chromium.org/12094056/diff/21001/tests/corelib/string_rune...
File tests/corelib/string_runes_test.dart (right):

https://codereview.chromium.org/12094056/diff/21001/tests/corelib/string_rune...
tests/corelib/string_runes_test.dart:64: test("\u{ffff}\u{10000}\u{10ffff}",
[0xffff, 0x10000, 0x10ffff]);
Can we have at least one test case where the 10 bits from the lead surrogate are
different from the 10 bits from the trail surrogate?

[floitsch, 2013-02-13 10:12:19]

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart
File sdk/lib/core/string.dart (right):

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:408: * Setting a negative [rawIndex], or one greater
than [:string.length:],
On 2013/02/12 15:13:23, Lasse Reichstein Nielsen wrote:
> I'd disallow string.length then. 
> 
> Positions in strings (or any sequence) are not of the characters, but of the
> fenceposts between characters.
*Nobody* except the implementors see it this way. I'm at a position and look at
the "current" character. Then you can move forward or backward. No user should
think of "between characters".

> That's why 0..length is allowed in arguments to String.substring.
And there it makes more sense, although most users see it as "from (including),
to (excluding)".
> 
> In this case, because we expect to do a moveNext immediately, I'm all for
> disallowing length.
> 
> And no, -1 is not the same as reset(0).

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:425: * Setting a negative [rawIndex], or one greater
than [:string.length:],
On 2013/02/12 15:13:23, Lasse Reichstein Nielsen wrote:
> For what?
> reset(0) is a reset to the beginning of the string.
> reset(string.length) positions you just after the last rune.
> It works here, like <deity> intended indices to work!

If I reset to (0) and then moveNext I don't expect to get a code-unit at
position 0. I just moved away from it!

Currently you allow to set length+1 positions. Basically to every code-unit, and
to the position after the last code-unit. What's missing is the position before
the first code-unit, so that a moveNext brings you to the first position.
And yes: I know you see it differently, but nobody else does.

[Lasse Reichstein Nielsen, 2013-02-13 17:20:42]

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart
File sdk/lib/core/string.dart (right):

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:425: * Setting a negative [rawIndex], or one greater
than [:string.length:],
On 2013/02/13 10:12:19, floitsch wrote:
> On 2013/02/12 15:13:23, Lasse Reichstein Nielsen wrote:
> > For what?
> > reset(0) is a reset to the beginning of the string.
> > reset(string.length) positions you just after the last rune.
> > It works here, like <deity> intended indices to work!
> 
> If I reset to (0) and then moveNext I don't expect to get a code-unit at
> position 0. I just moved away from it!

That's not how RuneIterator.reset works. 
Are you talking about assigning zero to rawIndex?

There are two ways to set a position:
- setting rawIndex of the character you want to be current.
Valid indices are 0 .. length - 1.
- resetting to a state with no current, but so that the character you want will
be current after a moveNext, or the previous one will be after a movePrevious.
Valid indices are 0 .. length.

This allows reset to position the iterator AT the end of the list (which can't
have a current value), but does not allow setting rawIndex there.

The reset functionality is essential if you need to do something to the
iterator, and then pass it to another function that expects to start with a
moveNext (which is likely to be the case). I like to have this functionality
generalized so that I can reset to any index without a current value - so reset
does that for me.

> 
> Currently you allow to set length+1 positions. Basically to every code-unit,
and
> to the position after the last code-unit. 
I changed that to only allowing 0..length - 1, i.e., only indices of actual code
units.

> What's missing is the position before
> the first code-unit, so that a moveNext brings you to the first position.

There is no "positions before the first". That is not a position. It has no
rawIndex (now that rawIndex returns null on a newly created iterator).

Setting the iterator so that moveNext hits the first rune, or movePrevious hits
the last rune, means that you can't use setting rawIndex for it, because that
always points to a rune.

I guess we can make special cases for that if you think it's worth it.

> And yes: I know you see it differently, but nobody else does.

Ok, then. How do you want setting rawIndex to work?

For 0..length - 1 it currently set the iterator to a state where current is the
rune starting at the index.

What do you want to happen for length?

What do you want to happen for -1?

Are there any other cases it should cover?

Just say so, and the CL will be in the mail :)

[floitsch, 2013-02-13 17:34:50]

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart
File sdk/lib/core/string.dart (right):

https://codereview.chromium.org/12094056/diff/15001/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:425: * Setting a negative [rawIndex], or one greater
than [:string.length:],
On 2013/02/13 17:20:42, Lasse Reichstein Nielsen wrote:
> On 2013/02/13 10:12:19, floitsch wrote:
> > On 2013/02/12 15:13:23, Lasse Reichstein Nielsen wrote:
> > > For what?
> > > reset(0) is a reset to the beginning of the string.
> > > reset(string.length) positions you just after the last rune.
> > > It works here, like <deity> intended indices to work!
> > 
> > If I reset to (0) and then moveNext I don't expect to get a code-unit at
> > position 0. I just moved away from it!
> 
> That's not how RuneIterator.reset works. 
> Are you talking about assigning zero to rawIndex?
> 
> There are two ways to set a position:
> - setting rawIndex of the character you want to be current.
> Valid indices are 0 .. length - 1.
> - resetting to a state with no current, but so that the character you want
will
> be current after a moveNext, or the previous one will be after a movePrevious.
> Valid indices are 0 .. length.
> 
> This allows reset to position the iterator AT the end of the list (which can't
> have a current value), but does not allow setting rawIndex there.
> 
> The reset functionality is essential if you need to do something to the
> iterator, and then pass it to another function that expects to start with a
> moveNext (which is likely to be the case). I like to have this functionality
> generalized so that I can reset to any index without a current value - so
reset
> does that for me.
> 
> > 
> > Currently you allow to set length+1 positions. Basically to every code-unit,
> and
> > to the position after the last code-unit. 
> I changed that to only allowing 0..length - 1, i.e., only indices of actual
code
> units.
> 
> > What's missing is the position before
> > the first code-unit, so that a moveNext brings you to the first position.
> 
> There is no "positions before the first". That is not a position. It has no
> rawIndex (now that rawIndex returns null on a newly created iterator).
> 
> Setting the iterator so that moveNext hits the first rune, or movePrevious
hits
> the last rune, means that you can't use setting rawIndex for it, because that
> always points to a rune.
> 
> I guess we can make special cases for that if you think it's worth it.
> 
> > And yes: I know you see it differently, but nobody else does.
> 
> Ok, then. How do you want setting rawIndex to work?
> 
> For 0..length - 1 it currently set the iterator to a state where current is
the
> rune starting at the index.
> 
> What do you want to happen for length?
> 
> What do you want to happen for -1?
> 
> Are there any other cases it should cover?
> 
> Just say so, and the CL will be in the mail :)

Ok for original behavior for reset (including allowing string.length).