On Thu, 25 Jan 2024 13:58:50 GMT, Tejesh R <[email protected]> wrote:
>> Suggested fix [JDK-8307091](https://bugs.openjdk.org/browse/JDK-8307091)
>> also created concurrent exception intermittently (monthly once/quarterly
>> once) on CI system. The issue was not able to be reproduced yet, hence
>> proposing an alternative fix which uses iterators to compare the List.
>> CI testing shows green.
>
> Tejesh R has updated the pull request with a new target base due to a merge
> or a rebase. The incremental webrev excludes the unrelated changes brought in
> by the merge/rebase. The pull request contains three additional commits since
> the last revision:
>
> - Revert fix 8307091 + Synchronised filecache
> - Merge branch 'master' of https://git.openjdk.java.net/jdk into
> branch_8323670
> - Fix
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java
line 1:
> 1: /*
Please update copyright year.
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java
line 32:
> 30: import java.beans.PropertyChangeSupport;
> 31: import java.io.File;
> 32: import java.util.Iterator;
Unused import.
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java
line 344:
> 342: // execute the whole block on the COM thread
> 343: runnable = ShellFolder.invoke(new
> Callable<DoChangeContents>() {
> 344: public DoChangeContents call() {
To reduce indentation level, you may convert the anonymous class to a lambda
expression or even to a method in `FilesLoader` in which case you could use
method reference.
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java
line 367:
> 365:
> 366: if (start >= 0 && end > start &&
> newFileCache.subList(end, newSize)
> 367: .equals(fileCache.subList(start,
> oldSize))) {
I suggest following the style used before your first fix:
Suggestion:
if (start >= 0 && end > start
&& newFileCache.subList(end, newSize)
.equals(fileCache.subList(start,
oldSize))) {
That is wrap at `&&` operator.
Then wrap before `.equals` to make the line fit into 80-column limit.
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java
line 372:
> 370: }
> 371: return new DoChangeContents(newFileCache
> 372: .subList(start, end), start,
> null, 0, fid);
I don't like such wrapping style. When you read this statement, you miss
`newFileCache`, you may think that the call to `subList` is on
`DoChangeContents`, which is not the case.
I suggest wrapping after the call to `subList`, that is after the first
parameter; or wrap before `newFileCache` so that all the parameters are on the
next line.
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java
line 387:
> 385:
> 386: if (start >= 0 && end > start &&
> fileCache.subList(end, oldSize)
> 387: .equals(newFileCache.subList(start,
> newSize))) {
The same as above:
Suggestion:
if (start >= 0 && end > start
&& fileCache.subList(end, oldSize)
.equals(newFileCache.subList(start,
newSize))) {
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java
line 392:
> 390: }
> 391: return new DoChangeContents(null, 0, new
> Vector<>(fileCache
> 392: .subList(start, end)), start,
> fid);
Wrapping at higher level is preferable:
Suggestion:
return new DoChangeContents(null, 0,
new Vector<>(fileCache.subList(start,
end)), start, fid);
-------------
PR Review: https://git.openjdk.org/jdk/pull/17462#pullrequestreview-1849605557
PR Review Comment: https://git.openjdk.org/jdk/pull/17462#discussion_r1470124921
PR Review Comment: https://git.openjdk.org/jdk/pull/17462#discussion_r1470101630
PR Review Comment: https://git.openjdk.org/jdk/pull/17462#discussion_r1470123895
PR Review Comment: https://git.openjdk.org/jdk/pull/17462#discussion_r1470107213
PR Review Comment: https://git.openjdk.org/jdk/pull/17462#discussion_r1470111897
PR Review Comment: https://git.openjdk.org/jdk/pull/17462#discussion_r1470117703
PR Review Comment: https://git.openjdk.org/jdk/pull/17462#discussion_r1470119698
