-
Notifications
You must be signed in to change notification settings - Fork 32
Conversation
LGTM overall, Lasse will do a style review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drive-by mostly-style comments.
lib/src/utf8.dart
Outdated
} | ||
|
||
static Pointer<Utf8> toUtf8(String s) { | ||
final List<int> units = Utf8Encoder().convert(s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe Utf8Encoder.convert
this is now typed to return Uint8List
. You may want to use that as type.
You can use const Utf8Encoder()
to avoid allocating a new object each time (if you care).
That's the same object returned by utf8.encoder
as well, and you could also just write utf8.encode(s)
for exactly the same result.
Easier to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the new type:
abstract class Encoding extends Codec<String, List<int>> {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some dbc
Working on better tests. |
PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM to me as well.
lib/src/utf8.dart
Outdated
int i = 0; | ||
while (nativeString[i] != 0) { | ||
if (++i == count) { | ||
count *= 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use a precise bound here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because the length of the string can be only be detected by scanning until the first NULL byte.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But, why times 2 every time? Why not scan until null and use that as count?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is scanning until null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I was confused by array.asExternalTypedData(count: count)
where that count is potentially 2x the string length. We need an oversized TypedData wrapper in order to not create a new wrapper every character for scanning in the first place. (I hope we can switch to the normal Pointer<Int8>.load()
soon, that would make it easier to read this code.)
No description provided.