Add String.replaceFirstMapped.

Add String.replaceFirstMapped.

Also refactory VM implementation of checking if a string is one-byte,
and adds dart2js implementation of replaceFirst for non-string/regexp pattern.

BUG= http://dartbug.com/2979
R=floitsch@google.com, iposva@google.com

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

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

[Lasse Reichstein Nielsen, 2015-02-11 11:23:43]

lrn@google.com changed reviewers:
+ floitsch@google.com, iposva@google.com

[floitsch, 2015-02-11 11:49:14]

LGTM, but wait for VM.

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

https://codereview.chromium.org/920453002/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:719: var matches = pattern.allMatches(this,
startIndex).iterator;
replace var with types.

https://codereview.chromium.org/920453002/diff/1/sdk/lib/_internal/compiler/j...
File sdk/lib/_internal/compiler/js_lib/string_helper.dart (right):

https://codereview.chromium.org/920453002/diff/1/sdk/lib/_internal/compiler/j...
sdk/lib/_internal/compiler/js_lib/string_helper.dart:198: var index =
receiver.indexOf(from, startIndex);
we are missing types here... (and in your code).

[Ivan Posva, 2015-02-11 22:45:40]

LGTM with comments addressed.

-Ivan

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

https://codereview.chromium.org/920453002/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:693: String replacement = "${replace(match)}";
Why this change? All it does is add string interpolation stuff around the
original code.

https://codereview.chromium.org/920453002/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:719: var matches = pattern.allMatches(this,
startIndex).iterator;
On 2015/02/11 11:49:14, floitsch wrote:
> replace var with types.

var is just fine in method bodies.

https://codereview.chromium.org/920453002/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:722: String replacement = "${replace(match)}";
ditto as in:
var replacement = replace(match).toString;

https://codereview.chromium.org/920453002/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:733: return _joinReplaceAllResult(this, slices,
length,
Why is there not the cutout for this being a one byte string and the resulting
length short enough and ..?
Same as above in replaceAllMapped?

[Lasse Reichstein Nielsen, 2015-02-12 09:03:57]

New patchsets have been uploaded after l-g-t-m from
floitsch@google.com,iposva@google.com

[Lasse Reichstein Nielsen, 2015-02-12 09:04:39]

Committed patchset #2 (id:20001) manually as 43716 (presubmit successful).

[Lasse Reichstein Nielsen, 2015-02-12 09:17:07]

Committed patchset #3 (id:40001) manually as 43718 (presubmit successful).

[Lasse Reichstein Nielsen, 2015-02-12 09:17:16]

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

https://codereview.chromium.org/920453002/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:693: String replacement = "${replace(match)}";
The interpolation also throws if the result of replace(match).toString() is not
a String. 
If the result of toString() is not a String, it is added to the list, and causes
an error later (likely when doing replacement.length below, but possibly even
later). So, to make errors more useful, I wanted to add an error here, and
rather than:
  var replacement = replace(match).toString();
  if (replacement is! String) throw new SameErrorAsInterpolation();
it might as well reuse the existing code.

https://codereview.chromium.org/920453002/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:719: var matches = pattern.allMatches(this,
startIndex).iterator;
Keeping as-is in VM libraries.

https://codereview.chromium.org/920453002/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:722: String replacement = "${replace(match)}";
Kept as interpolation, but changed "String" to "var".

https://codereview.chromium.org/920453002/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:733: return _joinReplaceAllResult(this, slices,
length,
Good point, will add.

The cut-off length might be worth dropping. It's likely that the Dart version is
faster for all lengths, but we should measure that.

https://codereview.chromium.org/920453002/diff/1/sdk/lib/_internal/compiler/j...
File sdk/lib/_internal/compiler/js_lib/string_helper.dart (right):

https://codereview.chromium.org/920453002/diff/1/sdk/lib/_internal/compiler/j...
sdk/lib/_internal/compiler/js_lib/string_helper.dart:198: var index =
receiver.indexOf(from, startIndex);
Added.
You are also missing types on all the function signatures, so I assumed it was
deliberate.

[sra1, 2015-02-17 17:00:59]

sra@google.com changed reviewers:
+ sra@google.com

[sra1, 2015-02-17 17:01:00]

https://codereview.chromium.org/920453002/diff/40001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/js_lib/string_helper.dart (right):

https://codereview.chromium.org/920453002/diff/40001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/string_helper.dart:201:
'${receiver.substring(index + from.length)}';
This common pattern (3x) makes me think that we should add

String replaceRange(int start, int end, String replacement)

(It is not quite a natural analog to List.replaceRange since that mutates, but
it is pretty close.)

We should certainly have a local unchecked static function version as the two
substring calls entail a lot of unnecessary validation.

WDYT?

[Lasse Reichstein Nielsen, 2015-02-23 08:39:17]

Re: replaceRange - I like it.