Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8351344: Avoid explicit Objects.requireNonNull in String.join #23710

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+2 −3
Open

8351344: Avoid explicit Objects.requireNonNull in String.join #23710

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4224314
[PATCH] Avoid explicit Objects.requireNonNull where we know implicit …
turbanoff Feb 20, 2025
0d03150
[PATCH] Avoid explicit Objects.requireNonNull where we know implicit …
turbanoff Feb 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/java.base/share/classes/java/lang/String.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1994, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1994, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -3645,11 +3645,10 @@ static String join(String prefix, String suffix, String delimiter, String[] elem
*/
public static String join(CharSequence delimiter,
Iterable<? extends CharSequence> elements) {
Objects.requireNonNull(delimiter);
Objects.requireNonNull(elements);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello Andrey, I have a different opinion about this. I think removing existing calls to Objects.requireNonNull() in favour of implicit null check (some times too far away in the code) isn't useful.

You noted that the implicit NullPointerException stacktrace are more user friendly now. I think you can get similar level of useful information from Objects.requireNonNull() too. In these two specific cases, what you could perhaps do is change those calls to even pass the message so that it's clear what parameter is null. Something like:

    Objects.requireNonNull(delimiter, "delimiter");
    Objects.requireNonNull(elements, "elements");

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add it to other overloads of join then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the easier to read explicit method calls to validate non-nullness should be left in place.

Hello Andrey, I have a different opinion about this. I think removing existing calls to Objects.requireNonNull() in favour of implicit null check (some times too far away in the code) isn't useful.

You noted that the implicit NullPointerException stacktrace are more user friendly now. I think you can get similar level of useful information from Objects.requireNonNull() too. In these two specific cases, what you could perhaps do is change those calls to even pass the message so that it's clear what parameter is null. Something like:

    Objects.requireNonNull(delimiter, "delimiter");
    Objects.requireNonNull(elements, "elements");

var delim = delimiter.toString();
var elems = new String[8];
int size = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a // implicit null check before the loop? Otherwise, the null check may be forgotten that future code change may early return before the null check here is triggered.

// implicit null check
for (CharSequence cs: elements) {
if (size >= elems.length) {
elems = Arrays.copyOf(elems, elems.length << 1);