Add String.replaceRange and use it in replaceFirst{,Mapped}.

Add String.replaceRange and use it in replaceFirst{,Mapped}.

R=floitsch@google.com

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

[Lasse Reichstein Nielsen, 2015-02-23 10:05:45]

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

[Lasse Reichstein Nielsen, 2015-02-23 10:05:46]

Seems like useful functionality since we implemented it twice already.

[floitsch, 2015-02-23 14:34:17]

LGTM.

[floitsch, 2015-02-23 14:36:35]

https://chromiumcodereview.appspot.com/949753005/diff/20001/sdk/lib/core/stri...
File sdk/lib/core/string.dart (right):

https://chromiumcodereview.appspot.com/949753005/diff/20001/sdk/lib/core/stri...
sdk/lib/core/string.dart:490: String replaceRange(String replacement, int start,
[int end]);
replaceRange in List takes the start-end first and the replacement as last
argument.
For consistency we should do the same here (even if that means that you cannot
omit 'end'.

[sra1, 2015-02-23 18:58:26]

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

[sra1, 2015-02-23 18:58:27]

DBC.

I agree with Florian on argument ordering.

https://codereview.chromium.org/949753005/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/js_lib/js_string.dart (right):

https://codereview.chromium.org/949753005/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_string.dart:107: return
"$prefix$replacement$suffix";
There are use cases for an unchecked version.

replace these three lines with:
return stringReplaceRangeUnchecked(this, start, end, replacement)
Add stringReplaceRangeUnchecked to string_helper.js

There are a couple of places in string_helper that could then call
stringReplaceRangeUnchecked, e.g. stringReplaceFirstUnchecked.

The cases that follow from String or RegExp are safe.
The cases that follow from a general Pattern might not be safe.

[kevmoo, 2015-02-23 19:14:23]

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

[kevmoo, 2015-02-23 19:14:23]

DBC

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

https://codereview.chromium.org/949753005/diff/20001/sdk/lib/core/string.dart...
sdk/lib/core/string.dart:490: String replaceRange(String replacement, int start,
[int end]);
On 2015/02/23 14:36:35, floitsch wrote:
> replaceRange in List takes the start-end first and the replacement as last
> argument.
> For consistency we should do the same here (even if that means that you cannot
> omit 'end'.

Could we look into named arguments?

Then someone could do just from a start
Or just to and end
Or omit the 'replacement' and delete the section in the range.

Optional positional paint us in a corner in future API changes

[sra1, 2015-02-23 20:35:37]

On 2015/02/23 19:14:23, kevmoo wrote:
> DBC
> 
> https://codereview.chromium.org/949753005/diff/20001/sdk/lib/core/string.dart
> File sdk/lib/core/string.dart (right):
> 
>
https://codereview.chromium.org/949753005/diff/20001/sdk/lib/core/string.dart...
> sdk/lib/core/string.dart:490: String replaceRange(String replacement, int
start,
> [int end]);
> On 2015/02/23 14:36:35, floitsch wrote:
> > replaceRange in List takes the start-end first and the replacement as last
> > argument.
> > For consistency we should do the same here (even if that means that you
cannot
> > omit 'end'.
> 
> Could we look into named arguments?
> 
> Then someone could do just from a start
> Or just to and end
> Or omit the 'replacement' and delete the section in the range.
> 
> Optional positional paint us in a corner in future API changes

Although 'just from a start' and 'just to an end' seem appealing, those cases
offer little benefit over substring and +:

str1.replaceRange(str2, start: start)     // implicit  end: str1.length
<=>
str1.substring(0, start) + str2

str1.replaceRange(str2, end: end)         // implicit  start: 0
<=>
str2 + str1.substring(end)


The main case, for replacing a substring is an expressiveness win (using
List.replaceRange order)

str1.replaceRange(start, end, str2)
<=>
"${str1.substring(0, start)}${str2}${str1.substring(end)}"

(The equivalent is does not use '+' to avoid a temporary string)

The boat has already sailed with List.replaceRange.
Since it is easier to not use replaceRange for call-sites where the intent is
the replacement of a prefix or suffix, I think String.replaceRange should keep
the same order of arguments as List.replaceRange.

Not having to process defaulted optional parameters helps address concerns with
there being too much checking in the code.

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

https://codereview.chromium.org/949753005/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/js_lib/js_string.dart (right):

https://codereview.chromium.org/949753005/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_string.dart:107: return
"$prefix$replacement$suffix";
On 2015/02/23 18:58:27, sra1 wrote:
> There are use cases for an unchecked version.
> 
> replace these three lines with:
> return stringReplaceRangeUnchecked(this, start, end, replacement)
> Add stringReplaceRangeUnchecked to string_helper.js
> 
> There are a couple of places in string_helper that could then call
> stringReplaceRangeUnchecked, e.g. stringReplaceFirstUnchecked.
> 
> The cases that follow from String or RegExp are safe.
> The cases that follow from a general Pattern might not be safe.
> 

Done.

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

https://codereview.chromium.org/949753005/diff/20001/sdk/lib/core/string.dart...
sdk/lib/core/string.dart:490: String replaceRange(String replacement, int start,
[int end]);
I agree that consistency wants to put the start/end arguments first (since it's
too late to fix the range functions, and we still don't have a slice operator).
Too late for named arguments (and they are annoying to use too).

As for painting us into a corner, please just fix the language, so it doesn't
have the annoying and arbitrary restriction against having both types of
optional parameters.

[Lasse Reichstein Nielsen, 2015-02-25 12:17:39]

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