[NEW BUG]: Avoid allocations in String.replace(CharSequence, CharSequence) in case no replacement happened

[Jonathan Bluett-Duncan]

Hi Christoph and Vitaly,

I am struggling to understand why the null-check for `replacement` has been
moved within the `if (j < 0) { ... }` block. Could one of you explain to me
the reasoning behind this change?

I ask because, AFAICT, making it so that it only throws an NPE if
`replacement` is null *and* `j < 0`, rather than throwing NPE at the very
start of the method, seems to be going against the class-level doc which
says, "Unless otherwise noted, passing a null argument to a constructor or
method in this class will cause a NullPointerException to be thrown."

Other than that, I am equally happy with `replacement` itself having an
explicit-nullness-related comment instead of target having an
implicit-nullness comment - it seems to deliver the same sort of message. :)

Kind regards,
Jonathan

On 2 March 2017 at 07:26, Christoph Dreis <christoph.dreis at freenet.de>
wrote:

> Hey Vitaly and Jonathan,
>
> > As mentioned offline, I would move the null check right before "return
> this".
>
> @Vitaly: Thanks again. Adjusted.
> @Jonathan: Thanks. Thought about that as well, but I'd probably rather go
> with explaining the explicit nullcheck.
>
> See the adjusted patch below. What do you think?
>
> ===== PATCH ======
> diff --git a/src/java.base/share/classes/java/lang/String.java
> b/src/java.base/share/classes/java/lang/String.java
> --- a/src/java.base/share/classes/java/lang/String.java
> +++ b/src/java.base/share/classes/java/lang/String.java
> @@ -2177,11 +2177,13 @@
>       */
>      public String replace(CharSequence target, CharSequence replacement) {
>          String tgtStr = target.toString();
> -        String replStr = replacement.toString();
>          int j = indexOf(tgtStr);
>          if (j < 0) {
> +            // Explicit nullcheck of replacement to fulfill NPE contract
> in early out
> +            Objects.requireNonNull(replacement);
>              return this;
>          }
> +        String replStr = replacement.toString();
>          int tgtLen = tgtStr.length();
>          int tgtLen1 = Math.max(tgtLen, 1);
>          int thisLen = length();
>
>