Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of [v3]
On Wed, 1 Feb 2023 23:50:07 GMT, Glavo wrote: >> `List.of` is cleaner, and can slightly reduce the memory footprint for lists >> of one or two elements. >> >> Because `List.of` can only store non-null elements, I have only replaced a >> few usage. >> >> Can someone open an Issue on JBS for me? > > Glavo has updated the pull request incrementally with one additional commit > since the last revision: > > reformat Marked as reviewed by nlisker (Reviewer). - PR: https://git.openjdk.org/jfx/pull/1012
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of [v3]
> `List.of` is cleaner, and can slightly reduce the memory footprint for lists > of one or two elements. > > Because `List.of` can only store non-null elements, I have only replaced a > few usage. > > Can someone open an Issue on JBS for me? Glavo has updated the pull request incrementally with one additional commit since the last revision: reformat - Changes: - all: https://git.openjdk.org/jfx/pull/1012/files - new: https://git.openjdk.org/jfx/pull/1012/files/33e22df6..4442eb09 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1012&range=02 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1012&range=01-02 Stats: 14 lines in 1 file changed: 2 ins; 6 del; 6 mod Patch: https://git.openjdk.org/jfx/pull/1012.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1012/head:pull/1012 PR: https://git.openjdk.org/jfx/pull/1012
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of [v2]
On Wed, 1 Feb 2023 23:38:57 GMT, Glavo wrote: >> `List.of` is cleaner, and can slightly reduce the memory footprint for lists >> of one or two elements. >> >> Because `List.of` can only store non-null elements, I have only replaced a >> few usage. >> >> Can someone open an Issue on JBS for me? > > Glavo has updated the pull request incrementally with two additional commits > since the last revision: > > - Update FileChooser > - Update getTracks You can put the argument list of the constructors and methods in one line. Also the `IllegalArgumentException` and its message can be in the same line. After you add the empty lines after the `FileChooser` class declaration and the `ExtensionFilter` class declaration this can go in. - PR: https://git.openjdk.org/jfx/pull/1012
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of [v2]
> `List.of` is cleaner, and can slightly reduce the memory footprint for lists > of one or two elements. > > Because `List.of` can only store non-null elements, I have only replaced a > few usage. > > Can someone open an Issue on JBS for me? Glavo has updated the pull request incrementally with two additional commits since the last revision: - Update FileChooser - Update getTracks - Changes: - all: https://git.openjdk.org/jfx/pull/1012/files - new: https://git.openjdk.org/jfx/pull/1012/files/b7482f9a..33e22df6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1012&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1012&range=00-01 Stats: 33 lines in 2 files changed: 1 ins; 23 del; 9 mod Patch: https://git.openjdk.org/jfx/pull/1012.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1012/head:pull/1012 PR: https://git.openjdk.org/jfx/pull/1012
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of
On Wed, 1 Feb 2023 16:54:30 GMT, John Hendrikx wrote:
>> modules/javafx.media/src/main/java/com/sun/media/jfxmedia/Media.java line
>> 103:
>>
>>> 101: }
>>> 102: }
>>> 103: return returnValue;
>>
>> This method can be reduced to
>>
>> public List getTracks() {
>> synchronized (tracks) {
>> return tracks.isEmpty() ? null : List.copyOf(tracks);
>> }
>> }
>>
>> though I find it highly questionable that it returns `null` for an empty
>> list instead of just an empty list. There are 2 use cases of this method and
>> both would do better with just an empty list.
>
> Yeah, I noticed this as well right away, it is documented to do this though.
> The documentation however does seem to suggest it might be possible that
> there are three results:
>
> 1. Tracks haven't been scanned yet -> `null`
> 2. Tracks have been scanned, but none where found -> empty list
> 3. Tracks have been scanned and some were found -> list
>
> Whether case 2 can ever happen is unclear, but distinguishing it from the
> case where nothing has been scanned yet with `null` does not seem
> unreasonable.
It's an internal class and no calling class makes this distinction. I don't
think it's meant to function in the way you described.
-
PR: https://git.openjdk.org/jfx/pull/1012
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of
On Wed, 1 Feb 2023 15:11:57 GMT, Glavo wrote: > I have considered this, but I didn't make this change because I was worried > that there would be less descriptive information when null was encountered. I think it's fine. The method is documented to throw and it happens immediately on entry. NPEs don't always have a message since they are straightforward. - PR: https://git.openjdk.org/jfx/pull/1012
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of
On Wed, 1 Feb 2023 14:36:51 GMT, Nir Lisker wrote:
>> `List.of` is cleaner, and can slightly reduce the memory footprint for lists
>> of one or two elements.
>>
>> Because `List.of` can only store non-null elements, I have only replaced a
>> few usage.
>>
>> Can someone open an Issue on JBS for me?
>
> modules/javafx.media/src/main/java/com/sun/media/jfxmedia/Media.java line 103:
>
>> 101: }
>> 102: }
>> 103: return returnValue;
>
> This method can be reduced to
>
> public List getTracks() {
> synchronized (tracks) {
> return tracks.isEmpty() ? null : List.copyOf(tracks);
> }
> }
>
> though I find it highly questionable that it returns `null` for an empty list
> instead of just an empty list. There are 2 use cases of this method and both
> would do better with just an empty list.
Yeah, I noticed this as well right away, it is documented to do this though.
The documentation however does seem to suggest it might be possible that there
are three results:
1. Tracks haven't been scanned yet -> `null`
2. Tracks have been scanned, but none where found -> empty list
3. Tracks have been scanned and some were found -> list
Whether case 2 can ever happen is unclear, but distinguishing it from the case
where nothing has been scanned yet with `null` does not seem unreasonable.
-
PR: https://git.openjdk.org/jfx/pull/1012
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of
On Thu, 26 Jan 2023 05:30:56 GMT, Glavo wrote:
> `List.of` is cleaner, and can slightly reduce the memory footprint for lists
> of one or two elements.
>
> Because `List.of` can only store non-null elements, I have only replaced a
> few usage.
>
> Can someone open an Issue on JBS for me?
> ### FileChooser
> `List.of` and `List.copyOf` already check for a `null` argument and `null`
> elements. This means that `validateArgs` only needs to check the `length` of
> `extensions` and for an empty element. Since the only difference between the
> constructors is taking an array vs. taking a list, once a list is created
> from the array, the array constructor can call the list constructor.
>
> I suggest the following refactoring:
>
> ```java
> public ExtensionFilter(String description, String... extensions) {
> this(description, List.of(extensions));
> }
>
> public ExtensionFilter(String description, List extensions) {
> var extensionsList = List.copyOf(extensions);
> validateArgs(description, extensionsList);
> this.description = description;
> this.extensions = extensionsList;
> }
> ```
>
> Note that `List.copyOf` will not create another copy if it was called from
> the array constructor that already created an unmodifiable `List.of`.
>
> Now validation can be reduced to
>
> ```java
> private static void validateArgs(String description, List
> extensions) {
> Objects.requireNonNull(description, "Description must not be
> null");
>
> if (description.isEmpty()) {
> throw new IllegalArgumentException("Description must not be
> empty");
> }
>
> if (extensions.isEmpty()) {
> throw new IllegalArgumentException("At least one extension
> must be defined");
> }
>
> for (String extension : extensions) {
> if (extension.isEmpty()) {
> throw new IllegalArgumentException("Extension must not be
> empty");
> }
> }
> }
> ```
>
> Aditionally, please add empty lines after the class declarations of
> `FileChooser` and `ExtensionFilter`.
>
> Also please correct the typo in `getTracks`: "The returned `List` **is**
> unmodifiable."
I have considered this, but I didn't make this change because I was worried
that there would be less descriptive information when null was encountered.
If this is not a problem, I am very happy to be able to carry out such
refactoring.
-
PR: https://git.openjdk.org/jfx/pull/1012
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of
On Thu, 26 Jan 2023 05:30:56 GMT, Glavo wrote:
> `List.of` is cleaner, and can slightly reduce the memory footprint for lists
> of one or two elements.
>
> Because `List.of` can only store non-null elements, I have only replaced a
> few usage.
>
> Can someone open an Issue on JBS for me?
### FileChooser
`List.of` and `List.copyOf` already check for a `null` argument and `null`
elements. This means that `validateArgs` only needs to check the `length` of
`extensions` and for an empty element. Since the only difference between the
constructors is taking an array vs. taking a list, once a list is created from
the array, the array constructor can call the list constructor.
I suggest the following refactoring:
public ExtensionFilter(String description, String... extensions) {
this(description, List.of(extensions));
}
public ExtensionFilter(String description, List extensions) {
var extensionsList = List.copyOf(extensions);
validateArgs(description, extensionsList);
this.description = description;
this.extensions = extensionsList;
}
Note that `List.copyOf` will not create another copy if it was called from the
array constructor that already created an unmodifiable `List.of`.
Now validation can be reduced to
private static void validateArgs(String description, List
extensions) {
Objects.requireNonNull(description, "Description must not be null");
if (description.isEmpty()) {
throw new IllegalArgumentException("Description must not be
empty");
}
if (extensions.isEmpty()) {
throw new IllegalArgumentException("At least one extension must
be defined");
}
for (String extension : extensions) {
if (extension.isEmpty()) {
throw new IllegalArgumentException("Extension must not be
empty");
}
}
}
Aditionally, please add empty lines after the class declarations of
`FileChooser` and `ExtensionFilter`.
Also please correct the typo in `getTracks`: "The returned List
**is** unmodifiable."
modules/javafx.graphics/src/main/java/com/sun/javafx/iio/common/ImageDescriptor.java
line 41:
> 39: this.extensions = List.of(extensions);
> 40: this.signatures = List.of(signatures);
> 41: this.mimeSubtypes = List.of(mimeSubtypes);
While this is not equivalent since changing the backing array will not change
the resulting list anymore, I would consider the old code a bug and this the
correct fix.
Note that in `FileChooser` care is taken to create a copy of the supplied array
before using it to create a list.
Additionally, care must be taken that all the callers don't have `null`
elements in the arrays they pass. I checked it and it's fine (and should
probably be disallowed, which is good now).
By the way, this class should be a `record`, and all its subtypes, which are
not really subtypes but just configured instances, should be modified
accordingly. This is out of scope though.
modules/javafx.media/src/main/java/com/sun/media/jfxmedia/Media.java line 100:
> 98: returnValue = null;
> 99: } else {
> 100: returnValue = List.copyOf(tracks);
This is fine because `addTrack` checks for `null` elements.
modules/javafx.media/src/main/java/com/sun/media/jfxmedia/Media.java line 103:
> 101: }
> 102: }
> 103: return returnValue;
This method can be reduced to
public List getTracks() {
synchronized (tracks) {
return tracks.isEmpty() ? null : List.copyOf(tracks);
}
}
though I find it highly questionable that it returns `null` for an empty list
instead of just an empty list. There are 2 use cases of this method and both
would do better with just an empty list.
-
Changes requested by nlisker (Reviewer).
PR: https://git.openjdk.org/jfx/pull/1012
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of
On Wed, 1 Feb 2023 11:21:04 GMT, Glavo wrote: >> Filed: https://bugs.openjdk.org/browse/JDK-8301604. Please let me know if >> the issue is not reported correctly. > > @theaoqi Thank you very much! @Glavo If you can't access JBS, you can submit a report via bugreport.java.com. @theaoqi If you create a JBS issue, fill in the affected version and fix version (`tbd` is fine) fields. The subcomponent is also not only graphics because there are changes to media as well. Use `other` for overarching changes like these (e.g., https://bugs.openjdk.org/browse/JDK-8208761). I can sponsor this fix, but it will need some more changes. Just a drop-in replacement with `List.of` doesn't result in good code. I will review it shortly. - PR: https://git.openjdk.org/jfx/pull/1012
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of
On Thu, 26 Jan 2023 05:30:56 GMT, Glavo wrote: > `List.of` is cleaner, and can slightly reduce the memory footprint for lists > of one or two elements. > > Because `List.of` can only store non-null elements, I have only replaced a > few usage. > > Can someone open an Issue on JBS for me? For a refactoring / cleanup fix like this, the question is whether it is worth the effort to test and review it. I don't know that it is, but if you can convince a Reviewer to test and review it, and then sponsor the fix for you, I won't object. - PR: https://git.openjdk.org/jfx/pull/1012
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of
On Wed, 1 Feb 2023 11:03:32 GMT, Ao Qi wrote: >> `List.of` is cleaner, and can slightly reduce the memory footprint for lists >> of one or two elements. >> >> Because `List.of` can only store non-null elements, I have only replaced a >> few usage. >> >> Can someone open an Issue on JBS for me? > > Filed: https://bugs.openjdk.org/browse/JDK-8301604. Please let me know if the > issue is not reported correctly. @theaoqi Thank you very much! - PR: https://git.openjdk.org/jfx/pull/1012
RFR: 8301604: Replace Collections.unmodifiableList with List.of
`List.of` is cleaner, and can slightly reduce the memory footprint for lists of one or two elements. Because `List.of` can only store non-null elements, I have only replaced a few usage. Can someone open an Issue on JBS for me? - Commit messages: - cleanup Changes: https://git.openjdk.org/jfx/pull/1012/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1012&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8301604 Stats: 18 lines in 3 files changed: 0 ins; 9 del; 9 mod Patch: https://git.openjdk.org/jfx/pull/1012.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1012/head:pull/1012 PR: https://git.openjdk.org/jfx/pull/1012
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of
On Thu, 26 Jan 2023 05:30:56 GMT, Glavo wrote: > `List.of` is cleaner, and can slightly reduce the memory footprint for lists > of one or two elements. > > Because `List.of` can only store non-null elements, I have only replaced a > few usage. > > Can someone open an Issue on JBS for me? Filed: https://bugs.openjdk.org/browse/JDK-8301604. Please let me know if the issue is not reported correctly. - PR: https://git.openjdk.org/jfx/pull/1012
