On 31.05.2015 18:54, Ivan Gerasimov wrote:
Hi Remi!
On 31.05.2015 11:43, Remi Forax wrote:
I agree with the others the code is more readable.
There is a slightly difference between the current behavior and the
behavior of the proposed code,
"foo".replace("bar", null)
should throw a NPE because replacement is null but the proposed code
will return "foo"
because replacement.toString() is called after the short-circuit test
(j < 0).
Yes, you're right, thanks for catching it!
And the regression test should have caught that, but I only had
"a".replace("a", null) there, which passed.
I fixed the code and added the case to the regression test in the new
webrev.
Which is right here:
http://cr.openjdk.java.net/~igerasim/8058779/05/webrev/
so the first part of the code should be:
String starget = target.toString(); // implict nullcheck
String srepl = replacement.toString(); // implicit nullcheck
int j = indexOf(starget);
if (j < 0) {
return this;
}
...
also 'i' can be initialized just before the 'do', there is no point
to initialize it before.
To finish, the two 'final' are useless here but i suppose it's a
matter of style :)
I moved declaration of i just to save a line. I don't think it
decreased performance.
Declaring the 'value' variable final was suggested by Martin, and I
think it is reasonable (see
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-March/032601.html).
The other variable is final just for symmetry :)
Sincerely yours,
Ivan
cheers,
Rémi
On 05/31/2015 04:19 AM, Ivan Gerasimov wrote:
Hi everyone!
Here's another webrev, in which replace() is implemented with
StringBuilder.
On my benchmark it is almost as fast as the version backed with
arrays, but this variant is much shorter.
Credits to Sherman for combining the general algorithm with the case
of empty target.
Comments, further suggestions are welcome!
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8058779
WEBREV: http://cr.openjdk.java.net/~igerasim/8058779/04/webrev/
Sincerely yours,
Ivan
