[logback-dev] svn commit: r1733 - in logback/trunk: logback-classic/src/main/java/ch/qos/logback/classic/jmx logback-core/src/main/java/ch/qos/logback/core logback-core/src/main/java/ch/qos/logback/core/status logback-core/src/main/java/ch/qos/logback/core/util logback-core/src/test/java/ch/qos/logback/core/joran/action logback-core/src/test/java/ch/qos/logback/core/status

[Ceki Gulcu]

Joern Huxhorn wrote:
> Hi Ceki,
> some comments on the latest commit:

> statusList should be final if it's later used for synchronization. At 
> As statusList, statusListenerList should be final.

OK.
>>  
>> -  public Iterator<Status> iterator() {
>> -    return statusList.iterator();
>> +  public synchronized List<Status> getCopyOfStatusList() {
>> +    synchronized (statusList) {
>> +      return new ArrayList<Status>(statusList);
>> +    }
>>    }
>>   
> I guess the method shouldn't be synchronized anymore since 
> synchronization is done on statusList.

 From what I can read from the ArrayList constructor taking a collection, 
copying of the collection passed as parameter is not synchronized. Moreover, the 
mutex used by the SyncronizedArrayList is the object itself (the 
SyncronizedArrayList instance), so synchronized(statusList){...} is necessary.

>> +  public void remove(StatusListener listener) {
>> +    statusListenerList.add(listener);
>> +  }
>>   
> Should be statusListenerList.remove(listener);

Four eyes are better than two. thank you!

> Regards,
> Joern.
> 

-- 
Ceki Gülcü