Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12088086:
Adapt String interface for Utf16. (Closed)
|
Created:
7 years, 10 months ago by floitsch Modified:
7 years, 10 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionAdapt String interface for Utf16.
Committed: https://code.google.com/p/dart/source/detail?r=18069
Reverted: https://code.google.com/p/dart/source/detail?r=18070
Committed: https://code.google.com/p/dart/source/detail?r=18072
Patch Set 1 #
Total comments: 48
Patch Set 2 : Address comments and implement missing methods. #
Total comments: 14
Patch Set 3 : Address comments. #Patch Set 4 : Rebase #Patch Set 5 : Don't change intrinsified String.charCodeAt. #
Messages
Total messages: 8 (0 generated)
Proposal for String interface change. Together with Lasse's rune-iterator the change itself should be relatively easy.
UTF-8 is a series of code units too, so "code units" is ambiguous. LGTM https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart File sdk/lib/core/string.dart (right): https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:9: * immutable. A string is represented by a list of 16-bit Unicode Unicode code units -> Unicode UTF-16 code units https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:11: * [codeUnits] members. Also accessible with [] https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:13: * Strings are encoded in Utf16. Decoding Utf16, which combines Correct spelling is "UTF-16". Our style guide and the limitations of valid identifiers mean it has to be called Utf16 in code, but in comments we can write it correctly. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:15: * terminology as Go we call Unicode code-points "runes". The 32-bit as -> to https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:22: * The [charCodes] can be code-units or runes. If a char-code value is code units -> UTF-16 code units https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:49: * Gets the character (as [String]) at the given [index]. Clarification: This is UTF_16 code-unit based and can result in a string with half of a surrogate pair. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:61: * Returns the code-unit (16-bit) at the given [index]. 16-bit -> 16 bit UTF-16 https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:68: * Returns the number of 16-bit code units in this string. The number code units -> UTF-16 code units https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:190: * Splitting with an empty string pattern (`""`) splits at code unit code unit -> UTF-16 code unit https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:198: * (meaning that a code-unit represents a rune) it is often better to a -> each https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:210: * want to split on rune boundaries, use [runes] and map the result. I feel this comment would benefit from an example: Iterable<String> characters = string.runes.map((c) => new String.fromCharCode(c)) https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:232: * Returns a list of 16-bit code-units of this string. code-units -> UTF-16 code units https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:239: * Returns an iterable of the 16-bit code-units of this string. And here https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:257: * where all characters are made lower case. Returns [:this:] otherwise. double space https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:262: * If this string is not already all uper case, returns a new string uper -> upper
lgtm https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart File sdk/lib/core/string.dart (right): https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:8: * The String class represents character strings. Strings are Newlines after first sentence. "strings" is not a common word. It's a "sequences of characters". https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:9: * immutable. A string is represented by a list of 16-bit Unicode "list" means something else in Dart. Again "sequence" might be appropriate. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:11: * [codeUnits] members. That returns a String, not a code unit. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:13: * Strings are encoded in Utf16. Decoding Utf16, which combines "The characters of a string are encoded as UTF-16 code units." The string itself is not encoded, it just is. Their content is stored in encoded form. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:14: * surrogate pairs, yields Unicode code-points. Following a similar No dash in "code points". It's two words. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:15: * terminology as Go we call Unicode code-points "runes". The 32-bit "we call X Y" puts two names right next to each other. How about "we use the name Y for X". https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:36: * The built string is of [length] one, if the [charCode] is less than "The new string contains a single code unit if the [charCode] can be represented in 16 bits. Otherwise .." (The current text compares values to bit-lengths). https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:89: bool operator ==(String other); Shouldn't this be bool operator==(Object other) to be compatible with Object.operator==? https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:252: // TODO(floitsch): make it a bidirectional iterator. Let's make it a Runes class with extra functionality on top of being a bidi-iterable. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:257: * where all characters are made lower case. Returns [:this:] otherwise. You need to say how upper-casing is done. ASCII only or Unicode case folding? In the latter case, in which locale? Does it normalize the result as well, or will it never do that? And we really need: int compareIgnoreCase(String other); too (which would use Unicode canonicalization instead of case folding).
Thank you both. PTAL. If you are ok with it, I would commit this version, and then we could add the missing functionality incrementally. Lasse, for example, has already something for the runes. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart File sdk/lib/core/string.dart (right): https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:8: * The String class represents character strings. Strings are On 2013/02/01 12:44:10, Lasse Reichstein Nielsen wrote: > Newlines after first sentence. > "strings" is not a common word. It's a "sequences of characters". Done. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:9: * immutable. A string is represented by a list of 16-bit Unicode On 2013/02/01 09:42:50, erikcorry wrote: > Unicode code units -> Unicode UTF-16 code units Done. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:9: * immutable. A string is represented by a list of 16-bit Unicode On 2013/02/01 12:44:10, Lasse Reichstein Nielsen wrote: > "list" means something else in Dart. Again "sequence" might be appropriate. Done. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:11: * [codeUnits] members. On 2013/02/01 09:42:50, erikcorry wrote: > Also accessible with [] reworded. PTAL. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:13: * Strings are encoded in Utf16. Decoding Utf16, which combines On 2013/02/01 09:42:50, erikcorry wrote: > Correct spelling is "UTF-16". Our style guide and the limitations of valid > identifiers mean it has to be called Utf16 in code, but in comments we can write > it correctly. Done. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:13: * Strings are encoded in Utf16. Decoding Utf16, which combines On 2013/02/01 12:44:10, Lasse Reichstein Nielsen wrote: > "The characters of a string are encoded as UTF-16 code units." > The string itself is not encoded, it just is. Their content is stored in encoded > form. Done. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:14: * surrogate pairs, yields Unicode code-points. Following a similar On 2013/02/01 12:44:10, Lasse Reichstein Nielsen wrote: > No dash in "code points". It's two words. Done. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:15: * terminology as Go we call Unicode code-points "runes". The 32-bit On 2013/02/01 12:44:10, Lasse Reichstein Nielsen wrote: > "we call X Y" puts two names right next to each other. How about "we use the > name Y for X". Done. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:22: * The [charCodes] can be code-units or runes. If a char-code value is On 2013/02/01 09:42:50, erikcorry wrote: > code units -> UTF-16 code units Done. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:36: * The built string is of [length] one, if the [charCode] is less than On 2013/02/01 12:44:10, Lasse Reichstein Nielsen wrote: > "The new string contains a single code unit if the [charCode] can be represented > in 16 bits. Otherwise .." > (The current text compares values to bit-lengths). Done. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:49: * Gets the character (as [String]) at the given [index]. On 2013/02/01 09:42:50, erikcorry wrote: > Clarification: This is UTF_16 code-unit based and can result in a string with > half of a surrogate pair. Reworded and added example. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:61: * Returns the code-unit (16-bit) at the given [index]. On 2013/02/01 09:42:50, erikcorry wrote: > 16-bit -> 16 bit UTF-16 Done. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:68: * Returns the number of 16-bit code units in this string. The number On 2013/02/01 09:42:50, erikcorry wrote: > code units -> UTF-16 code units Done. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:89: bool operator ==(String other); On 2013/02/01 12:44:10, Lasse Reichstein Nielsen wrote: > Shouldn't this be > bool operator==(Object other) > to be compatible with Object.operator==? yes. Changed to "var". https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:190: * Splitting with an empty string pattern (`""`) splits at code unit On 2013/02/01 09:42:50, erikcorry wrote: > code unit -> UTF-16 code unit Done. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:198: * (meaning that a code-unit represents a rune) it is often better to On 2013/02/01 09:42:50, erikcorry wrote: > a -> each Done. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:210: * want to split on rune boundaries, use [runes] and map the result. On 2013/02/01 09:42:50, erikcorry wrote: > I feel this comment would benefit from an example: > Iterable<String> characters = string.runes.map((c) => new > String.fromCharCode(c)) Shouldn't be necessary since the method is going away soon. But done. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:232: * Returns a list of 16-bit code-units of this string. On 2013/02/01 09:42:50, erikcorry wrote: > code-units -> UTF-16 code units Done. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:239: * Returns an iterable of the 16-bit code-units of this string. On 2013/02/01 09:42:50, erikcorry wrote: > And here Done. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:252: // TODO(floitsch): make it a bidirectional iterator. On 2013/02/01 12:44:10, Lasse Reichstein Nielsen wrote: > Let's make it a Runes class with extra functionality on top of being a > bidi-iterable. Changed TODO. I will see that I can commit this CL eventually, and you can then replace the "Unimplemented" with the class you wrote. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:257: * where all characters are made lower case. Returns [:this:] otherwise. On 2013/02/01 09:42:50, erikcorry wrote: > double space Done. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:257: * where all characters are made lower case. Returns [:this:] otherwise. On 2013/02/01 12:44:10, Lasse Reichstein Nielsen wrote: > You need to say how upper-casing is done. ASCII only or Unicode case folding? In > the latter case, in which locale? Same as JavaScript: using the locale-independent Unicode mapping. > Does it normalize the result as well, or will it never do that? > > And we really need: > int compareIgnoreCase(String other); > too (which would use Unicode canonicalization instead of case folding). Added TODO. https://codereview.chromium.org/12088086/diff/1/sdk/lib/core/string.dart#newc... sdk/lib/core/string.dart:262: * If this string is not already all uper case, returns a new string On 2013/02/01 09:42:50, erikcorry wrote: > uper -> upper Done.
LGTM https://codereview.chromium.org/12088086/diff/6001/sdk/lib/core/string.dart File sdk/lib/core/string.dart (right): https://codereview.chromium.org/12088086/diff/6001/sdk/lib/core/string.dart#n... sdk/lib/core/string.dart:16: * terminology to Go we use the name "rune" for a Unicode code point. The 32-bit What makes them 32-bit? In Dart, integers do not have a bit-size, and in reality they are 21 bit integers. Instead say, modify the previous sentence: .. we use the name "rune" for an integer representing a Unicode code point. The runes of a string are accessible through the [runes] getter. https://codereview.chromium.org/12088086/diff/6001/sdk/lib/core/string.dart#n... sdk/lib/core/string.dart:38: * represented in 16 bits. Otherwise the [length] is 2 and the code units form ... represented by a single UTF-16 code unit. In general, try to avoid talking about bits. https://codereview.chromium.org/12088086/diff/6001/sdk/lib/core/string.dart#n... sdk/lib/core/string.dart:44: factory String.fromCharCode(int charCode) { Should this be ..(int rune) ? https://codereview.chromium.org/12088086/diff/6001/sdk/lib/core/string.dart#n... sdk/lib/core/string.dart:54: * musical g clef ("𝄞") with rune value 0x1D11E consists of a UTF-16 surrogate g clef -> G-clef https://codereview.chromium.org/12088086/diff/6001/sdk/lib/core/string.dart#n... sdk/lib/core/string.dart:225: * Returns a list of the individual code-units characters of this string. code-units -> code-unit. Or? Not sure I understand the sentence, even if I know what the method does. https://codereview.chromium.org/12088086/diff/6001/sdk/lib/core/string.dart#n... sdk/lib/core/string.dart:254: * Returns a list of 16-bit UTF-16 code units of this string. Drop 16-bit. That's what UTF-16 code units are. https://codereview.chromium.org/12088086/diff/6001/sdk/lib/core/string.dart#n... sdk/lib/core/string.dart:261: * Returns an iterable of the 16-bit UTF-16 code units of this string. drop 16-bit.
https://codereview.chromium.org/12088086/diff/6001/sdk/lib/core/string.dart File sdk/lib/core/string.dart (right): https://codereview.chromium.org/12088086/diff/6001/sdk/lib/core/string.dart#n... sdk/lib/core/string.dart:16: * terminology to Go we use the name "rune" for a Unicode code point. The 32-bit On 2013/02/04 15:26:01, Lasse Reichstein Nielsen wrote: > What makes them 32-bit? > In Dart, integers do not have a bit-size, and in reality they are 21 bit > integers. > > Instead say, modify the previous sentence: > .. we use the name "rune" for an integer representing a > Unicode code point. The runes of a string are accessible > through the [runes] getter. Done. https://codereview.chromium.org/12088086/diff/6001/sdk/lib/core/string.dart#n... sdk/lib/core/string.dart:38: * represented in 16 bits. Otherwise the [length] is 2 and the code units form On 2013/02/04 15:26:01, Lasse Reichstein Nielsen wrote: > ... represented by a single UTF-16 code unit. > In general, try to avoid talking about bits. Done. https://codereview.chromium.org/12088086/diff/6001/sdk/lib/core/string.dart#n... sdk/lib/core/string.dart:44: factory String.fromCharCode(int charCode) { On 2013/02/04 15:26:01, Lasse Reichstein Nielsen wrote: > Should this be ..(int rune) ? Not necessarily, since it can contain half of a surrogate pair. https://codereview.chromium.org/12088086/diff/6001/sdk/lib/core/string.dart#n... sdk/lib/core/string.dart:54: * musical g clef ("𝄞") with rune value 0x1D11E consists of a UTF-16 surrogate On 2013/02/04 15:26:01, Lasse Reichstein Nielsen wrote: > g clef -> G-clef > Done. https://codereview.chromium.org/12088086/diff/6001/sdk/lib/core/string.dart#n... sdk/lib/core/string.dart:225: * Returns a list of the individual code-units characters of this string. On 2013/02/04 15:26:01, Lasse Reichstein Nielsen wrote: > code-units -> code-unit. > Or? Not sure I understand the sentence, even if I know what the method does. Changed to: Returns a list of the individual code-units converted to strings. https://codereview.chromium.org/12088086/diff/6001/sdk/lib/core/string.dart#n... sdk/lib/core/string.dart:254: * Returns a list of 16-bit UTF-16 code units of this string. On 2013/02/04 15:26:01, Lasse Reichstein Nielsen wrote: > Drop 16-bit. That's what UTF-16 code units are. Done. https://codereview.chromium.org/12088086/diff/6001/sdk/lib/core/string.dart#n... sdk/lib/core/string.dart:261: * Returns an iterable of the 16-bit UTF-16 code units of this string. On 2013/02/04 15:26:01, Lasse Reichstein Nielsen wrote: > drop 16-bit. Done.
TBR after fix (just keeping charCodeAt unmodified and redirecting codeUnitAt to it, instead of the other way around).
Message was sent while issue was closed.
Please CC me always when making changes in runtime/lib Thanks, |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
