On Wed, 6 Mar 2024 10:43:16 GMT, Tejesh R <t...@openjdk.org> wrote:
>> Alexey Ivanov has updated the pull request incrementally with four
>> additional commits since the last revision:
>>
>> - Replace synchronized invalidateFileCache with synchronized block inside
>> - Declare DoChangeContents constructor private, wrap its parameters
>> - Space after synchronized in DoChangeContents.run
>> - Convert runnable to local variable
>
> src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java
> line 552:
>
>> 550: if (remSize > 0 && addSize == 0) {
>> 551: fireIntervalRemoved(BasicDirectoryModel.this, remStart,
>> remStart + remSize - 1);
>> 552: } else if (addSize > 0 && remSize == 0 && addStart +
>> addSize <= fileCache.size()) {
>
> Any reason for moving this portion out of `Synchronized` block? Because
> `fileCache.size()` might need to be inside the `Synchronized` block right?
I didn't touch it, it never was inside the synchronized block. Only indentation
was changed in this method.
However, you're right it's better to store the size of `fileCache` inside the
synchronized block. The lock should not be held while notifying listeners.
On the other hand, `fileCache` can't change. The only place where `fileCache`
is modified is above in this method, and only on EDT. The EDT will remain busy
until a `fire*` method returns.
I've updated the code to avoid any confusion. Thanks!
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18111#discussion_r1514311500
