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.dr...@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();
>
>
