You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by sc...@apache.org on 2009/10/26 22:56:47 UTC

svn commit: r829991 - /commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/ToStringBuilder.java

Author: scolebourne
Date: Mon Oct 26 21:56:46 2009
New Revision: 829991

URL: http://svn.apache.org/viewvc?rev=829991&view=rev
Log:
LANG-487 - Make default style thread-safe

Modified:
    commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/ToStringBuilder.java

Modified: commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/ToStringBuilder.java
URL: http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/ToStringBuilder.java?rev=829991&r1=829990&r2=829991&view=diff
==============================================================================
--- commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/ToStringBuilder.java (original)
+++ commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/ToStringBuilder.java Mon Oct 26 21:56:46 2009
@@ -94,27 +94,51 @@
     /**
      * The default style of output to use.
      */
-    private static ToStringStyle defaultStyle = ToStringStyle.DEFAULT_STYLE;
+    private static volatile ToStringStyle defaultStyle = ToStringStyle.DEFAULT_STYLE;
 
     //----------------------------------------------------------------------------
 
     /**
      * <p>Gets the default <code>ToStringStyle</code> to use.</p>
-     *
-     * <p>This could allow the <code>ToStringStyle</code> to be
-     * controlled for an entire application with one call.</p>
-     *
-     * <p>This might be used to have a verbose
-     * <code>ToStringStyle</code> during development and a compact
-     * <code>ToStringStyle</code> in production.</p>
      * 
-     * @return the default <code>ToStringStyle</code>
+     * <p>This method gets a singleton default value, typically for the whole JVM.
+     * Changing this default should generally only be done during application startup.
+     * It is recommended to pass a <code>ToStringStyle</code> to the constructor instead
+     * of using this global default.</p>
+     * 
+     * <p>This method is thread-safe, as a <code>volatile</code variable is used internally.</p>
+     * 
+     * <p>One reason for changing the default could be to have a verbose style during
+     * development and a compact style in production.</p>
+     * 
+     * @return the default <code>ToStringStyle</code>, never null
      */
     public static ToStringStyle getDefaultStyle() {
         return defaultStyle;
     }
 
     /**
+     * <p>Sets the default <code>ToStringStyle</code> to use.</p>
+     * 
+     * <p>This method sets a singleton default value, typically for the whole JVM.
+     * Changing this default should generally only be done during application startup.
+     * It is recommended to pass a <code>ToStringStyle</code> to the constructor instead
+     * of changing this global default.</p>
+     * 
+     * <p>This method is thread-safe, as a <code>volatile</code variable is used internally.</p>
+     * 
+     * @param style  the default <code>ToStringStyle</code>
+     * @throws IllegalArgumentException if the style is <code>null</code>
+     */
+    public static void setDefaultStyle(ToStringStyle style) {
+        if (style == null) {
+            throw new IllegalArgumentException("The style must not be null");
+        }
+        defaultStyle = style;
+    }
+
+    //----------------------------------------------------------------------------
+    /**
      * <p>Forwards to <code>ReflectionToStringBuilder</code>.</p>
      * 
      * @param object  the Object to be output
@@ -169,18 +193,7 @@
         return ReflectionToStringBuilder.toString(object, style, outputTransients, false, reflectUpToClass);
     }
 
-    /**
-     * <p>Sets the default <code>ToStringStyle</code> to use.</p>
-     * 
-     * @param style  the default <code>ToStringStyle</code>
-     * @throws IllegalArgumentException if the style is <code>null</code>
-     */
-    public static void setDefaultStyle(ToStringStyle style) {
-        if (style == null) {
-            throw new IllegalArgumentException("The style must not be null");
-        }
-        defaultStyle = style;
-    }
+    //----------------------------------------------------------------------------
 
     /**
      * Current toString buffer.
@@ -208,7 +221,7 @@
      *  <code>null</code>
      */
     public ToStringBuilder(Object object) {
-        this(object, getDefaultStyle(), null);
+        this(object, null, null);
     }
 
     /**



Re: svn commit: r829991 - /commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/ToStringBuilder.java

Posted by Stephen Colebourne <sc...@btopenworld.com>.
sebb wrote:
>>  +     * <p>This method is thread-safe, as a <code>volatile</code variable is used internally.</p>
> 
> I think it's misleading to call the method thread-safe.
> Adding volatile/synch. merely ensures that the new value is published correctly.
> 
> The use of a mutable static variable means that the method cannot
> really be considered thread-safe. Two independent threads cannot - in
> general - both use the set method and still be assured of predictable
> behaviour.

Reworded.
Stephen

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Re: svn commit: r829991 - /commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/ToStringBuilder.java

Posted by sebb <se...@gmail.com>.
On 26/10/2009, scolebourne@apache.org <sc...@apache.org> wrote:
> Author: scolebourne
>  Date: Mon Oct 26 21:56:46 2009
>  New Revision: 829991
>
>  URL: http://svn.apache.org/viewvc?rev=829991&view=rev
>  Log:
>  LANG-487 - Make default style thread-safe
>
>  Modified:
>     commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/ToStringBuilder.java
>
>  Modified: commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/ToStringBuilder.java
>  URL: http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/ToStringBuilder.java?rev=829991&r1=829990&r2=829991&view=diff
>  ==============================================================================
>  --- commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/ToStringBuilder.java (original)
>  +++ commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/ToStringBuilder.java Mon Oct 26 21:56:46 2009
>  @@ -94,27 +94,51 @@
>      /**
>       * The default style of output to use.
>       */
>  -    private static ToStringStyle defaultStyle = ToStringStyle.DEFAULT_STYLE;
>  +    private static volatile ToStringStyle defaultStyle = ToStringStyle.DEFAULT_STYLE;
>
>      //----------------------------------------------------------------------------
>
>      /**
>       * <p>Gets the default <code>ToStringStyle</code> to use.</p>
>  -     *
>  -     * <p>This could allow the <code>ToStringStyle</code> to be
>  -     * controlled for an entire application with one call.</p>
>  -     *
>  -     * <p>This might be used to have a verbose
>  -     * <code>ToStringStyle</code> during development and a compact
>  -     * <code>ToStringStyle</code> in production.</p>
>       *
>  -     * @return the default <code>ToStringStyle</code>
>  +     * <p>This method gets a singleton default value, typically for the whole JVM.
>  +     * Changing this default should generally only be done during application startup.
>  +     * It is recommended to pass a <code>ToStringStyle</code> to the constructor instead
>  +     * of using this global default.</p>
>  +     *

Agreed.

>  +     * <p>This method is thread-safe, as a <code>volatile</code variable is used internally.</p>

I think it's misleading to call the method thread-safe.
Adding volatile/synch. merely ensures that the new value is published correctly.

The use of a mutable static variable means that the method cannot
really be considered thread-safe. Two independent threads cannot - in
general - both use the set method and still be assured of predictable
behaviour.

>  +     *
>  +     * <p>One reason for changing the default could be to have a verbose style during
>  +     * development and a compact style in production.</p>
>  +     *
>  +     * @return the default <code>ToStringStyle</code>, never null
>       */
>      public static ToStringStyle getDefaultStyle() {
>          return defaultStyle;
>      }
>
>      /**
>  +     * <p>Sets the default <code>ToStringStyle</code> to use.</p>
>  +     *
>  +     * <p>This method sets a singleton default value, typically for the whole JVM.
>  +     * Changing this default should generally only be done during application startup.
>  +     * It is recommended to pass a <code>ToStringStyle</code> to the constructor instead
>  +     * of changing this global default.</p>
>  +     *
>  +     * <p>This method is thread-safe, as a <code>volatile</code variable is used internally.</p>

DItto.

>  +     *
>  +     * @param style  the default <code>ToStringStyle</code>
>  +     * @throws IllegalArgumentException if the style is <code>null</code>
>  +     */
>  +    public static void setDefaultStyle(ToStringStyle style) {
>  +        if (style == null) {
>  +            throw new IllegalArgumentException("The style must not be null");
>  +        }
>  +        defaultStyle = style;
>  +    }
>  +
>  +    //----------------------------------------------------------------------------
>  +    /**
>       * <p>Forwards to <code>ReflectionToStringBuilder</code>.</p>
>       *
>       * @param object  the Object to be output
>  @@ -169,18 +193,7 @@
>          return ReflectionToStringBuilder.toString(object, style, outputTransients, false, reflectUpToClass);
>      }
>
>  -    /**
>  -     * <p>Sets the default <code>ToStringStyle</code> to use.</p>
>  -     *
>  -     * @param style  the default <code>ToStringStyle</code>
>  -     * @throws IllegalArgumentException if the style is <code>null</code>
>  -     */
>  -    public static void setDefaultStyle(ToStringStyle style) {
>  -        if (style == null) {
>  -            throw new IllegalArgumentException("The style must not be null");
>  -        }
>  -        defaultStyle = style;
>  -    }
>  +    //----------------------------------------------------------------------------
>
>      /**
>       * Current toString buffer.
>  @@ -208,7 +221,7 @@
>       *  <code>null</code>
>       */
>      public ToStringBuilder(Object object) {
>  -        this(object, getDefaultStyle(), null);
>  +        this(object, null, null);
>      }
>
>      /**
>
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org