Avoid unnecessary copying when parsing doubles.

Avoid unnecessary copying when parsing doubles.

R=sgjesse@google.com, vegorov@google.com

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

[Søren Gjesse, 2014-07-01 08:19:40]

LGTM

[Vyacheslav Egorov (Google), 2014-07-01 11:36:18]

lgtm

https://codereview.chromium.org/368483004/diff/80001/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://codereview.chromium.org/368483004/diff/80001/runtime/lib/double.cc#ne...
runtime/lib/double.cc:195: // The native function only accepts one-byte strings.
The Dart side converts
This comment does not seem to be valid anymore.

https://codereview.chromium.org/368483004/diff/80001/runtime/lib/double_patch...
File runtime/lib/double_patch.dart (right):

https://codereview.chromium.org/368483004/diff/80001/runtime/lib/double_patch...
runtime/lib/double_patch.dart:10: static double _native_parse(String str,
Should not this be called _nativeParse? _native_parse violates the style guide.

Also instead of double linking to it from double._native_parse and _JsonParser
could we have moved it to dart:_internal?

[Lasse Reichstein Nielsen, 2014-07-01 11:54:18]

https://codereview.chromium.org/368483004/diff/80001/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://codereview.chromium.org/368483004/diff/80001/runtime/lib/double.cc#ne...
runtime/lib/double.cc:195: // The native function only accepts one-byte strings.
The Dart side converts
Well spotted.

https://codereview.chromium.org/368483004/diff/80001/runtime/lib/double_patch...
File runtime/lib/double_patch.dart (right):

https://codereview.chromium.org/368483004/diff/80001/runtime/lib/double_patch...
runtime/lib/double_patch.dart:10: static double _native_parse(String str,
Changed to _nativeParse.

Moving it to dart:_internal would mean that it's also visible to dart2js, which
we don't want.

It might be possible to add a function in the internal_patch.dart file, but it
violates the way patch files are *supposed* to work (only add private names or
patch external names). I'd prefer not to do that.

[Lasse Reichstein Nielsen, 2014-07-01 11:59:40]

Committed patchset #6 manually as r37863 (presubmit successful).

[Ivan Posva, 2014-07-03 19:44:14]

Lasse,

Thanks for addressing this. Please fix the types and review the backward
scanning for whitespace.

-Ivan

https://codereview.chromium.org/368483004/diff/90001/runtime/lib/string_patch...
File runtime/lib/string_patch.dart (right):

https://codereview.chromium.org/368483004/diff/90001/runtime/lib/string_patch...
runtime/lib/string_patch.dart:309: for (; last >= 0; last--) {
I know this was used in trim before, but I think it predates the switch away
from 4-byte string encodings. Is it legal to scan backwards for white space with
the current use of surrogate pairs?

https://codereview.chromium.org/368483004/diff/90001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/368483004/diff/90001/runtime/vm/object.cc#new...
runtime/vm/object.cc:16916: int length = end - start;
Please always use intptr_t.

https://codereview.chromium.org/368483004/diff/90001/runtime/vm/object.cc#new...
runtime/vm/object.cc:16925: for (int i = 0; i < length; i++) {
intptr_t

https://codereview.chromium.org/368483004/diff/90001/runtime/vm/object.cc#new...
runtime/vm/object.cc:16926: int ch = str.CharAt(start + i);
The return type of CharAt is int32_t.

Also it would be better to get the CharAt function using CharAtFunc once outside
the loop to avoid the repeated virtual dispatch.

[Lasse Reichstein Nielsen, 2014-07-04 06:46:01]

https://codereview.chromium.org/368483004/diff/90001/runtime/lib/string_patch...
File runtime/lib/string_patch.dart (right):

https://codereview.chromium.org/368483004/diff/90001/runtime/lib/string_patch...
runtime/lib/string_patch.dart:283: static bool _isTwoByteWhitespace(int
codePoint) {
The "codePoint" argument is really a "codeUnit". I'll rename it for clarity.

https://codereview.chromium.org/368483004/diff/90001/runtime/lib/string_patch...
runtime/lib/string_patch.dart:309: for (; last >= 0; last--) {
Yes. All Unicode white-space are in the basic multilingual plane, so no
surrogate is part of a white-space character.

https://codereview.chromium.org/368483004/diff/90001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/368483004/diff/90001/runtime/vm/object.cc#new...
runtime/vm/object.cc:16916: int length = end - start;
Ack, yes. Will do.

https://codereview.chromium.org/368483004/diff/90001/runtime/vm/object.cc#new...
runtime/vm/object.cc:16926: int ch = str.CharAt(start + i);
Will do.
Didn't know about the CharAtFunc - nice!