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

[Paul Sandoz]

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     }


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}

No need for another review for any of these.

Paul.