[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

[Joern Huxhorn]

Hi Ceki,
some comments on the latest commit:

noreply.ceki at qos.ch wrote:
> Modified: logback/trunk/logback-core/src/main/java/ch/qos/logback/core/BasicStatusManager.java
> ==============================================================================
> --- logback/trunk/logback-core/src/main/java/ch/qos/logback/core/BasicStatusManager.java	(original)
> +++ logback/trunk/logback-core/src/main/java/ch/qos/logback/core/BasicStatusManager.java	Wed Aug  6 18:51:10 2008
> @@ -10,25 +10,39 @@
>   
[..]
> +  // This method is synchronized on the instance.
> +  // Code
>    int count = 0;
> -  List<Status> statusList = new ArrayList<Status>();
> +  
> +  // reading SynchronizedCollection the mutex is the returned 
> +  // synchronized list, we make use of this fact in getCopyOfStatusList
> +  List<Status> statusList = Collections
> +      .synchronizedList(new ArrayList<Status>());
>   
statusList should be final if it's later used for synchronization. At 
least that's what IDEA says...

>    int level = Status.INFO;
>  
> -  // This method is synchronized on the instance.
> -  // Code iterating on this.iterator is expected to
> -  // also synchronize on this (the BasicStatusManager instance)
> -  public synchronized void add(Status newStatus) {
> -    // System.out.println(newStatus);
> +  // reading SynchronizedCollection the mutex is the returned 
> +  // synchronized list, we make use of this fact in getCopyOfStatusListnerList
> +  List<StatusListener> statusListenerList = Collections
> +      .synchronizedList(new ArrayList<StatusListener>());
>   
As statusList, statusListenerList should be final.
> +
> +  /**
> +   * Add a new status object.
> +   * 
> +   * @param Status
> +   *                the status message to add
> +   */
> +  public void add(Status newStatus) {
>      if (count > MAX_COUNT) {
>        return;
>      }
> @@ -40,8 +54,10 @@
>      statusList.add(newStatus);
>    }
>  
> -  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.

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

Regards,
Joern.