8058779: Faster implementation of String.replace(CharSequence, CharSequence)

[Ivan Gerasimov]

On 02.06.2015 11:20, Paul Sandoz wrote:
> On Jun 1, 2015, at 8:53 PM, Ivan Gerasimov <ivan.gerasimov at oracle.com> wrote:
>
>>
>> On 01.06.2015 11:33, Paul Sandoz wrote:
>>> On May 31, 2015, at 6:03 PM, Ivan Gerasimov <ivan.gerasimov at oracle.com> wrote:
>>>> Which is right here:
>>>> http://cr.openjdk.java.net/~igerasim/8058779/05/webrev/
>>>>
>>> Much better.
>>>
>>> For the test can you use RandomFactory recently added to the test library?
>> Sure.
>> Here the updated webrev with this change and a few other minor changes.
>> http://cr.openjdk.java.net/~igerasim/8058779/06/webrev/
>>
>> The changes are:
>> - move declaration of i below,
>> - indent .append(),
>> - use RandomFactory in the test,
>> - extend number of test cases with null input.
>>
>> Do you think it's ready to go?
>>
> Almost :-)
>
>
> Like Sherman i think you should get rid of JavaLangAccess.newStringUnsafe.
>
>
>    78     @Test(dataProvider="sourceTargetReplacementWithNull")
>
> You can use the "expectedExceptions" attribute instead of an explicit try/fail/catch.
>
>    79     public void testNPE(String source, String target, String replacement) {
>    80         try {
>    81             source.replace(target, replacement);
>    82             fail("Expected to throw NPE: " + source + ".replace(" + target +
>    83                     "," + replacement + ")");
>    84         } catch (NullPointerException npe) {
>    85         }
>    86     }

Alright, I will use expectedExceptions here.
>
> You could simplify the data provider sourceTargetReplacementWithNull, there is no point testing with a null source. String.replace accepts two arguments, each can be either null or non-null. Thats 2 bits of state, so one can simply be explicit and presumably it should not matter what the non-null argument value (there are enough non-null values supplied by the other data providers):
>
>    {null, null}
>    {null, "foo"}
>    {"foo", null}
I agree there is not much value in testing the case source == null, so 
it can be removed.
However, I would leave other test cases.
Earlier in this thread Rémi spotted a bug, which had been observed in a 
call "bar".replace("foo", null), but not in "foo".replace("foo", null).

That was my motivation to extend the number of test cases for null inputs.

Sincerely yours,
Ivan

> No need for another review for any of these.
>
> Paul.
>
>