Updated in 1589555.
On 23 April 2014 16:51, Matt Sicker <[email protected]> wrote: > Will do. The important thing is to add the @throws javadoc for runtime > exceptions (especially for log4j-api). > > > On 23 April 2014 09:59, Ralph Goers <[email protected]> wrote: > >> I didn’t realize you had changed IAEs I coded to NPEs. Please change them >> back. >> >> Ralph >> >> On Apr 23, 2014, at 8:53 AM, Ralph Goers <[email protected]> >> wrote: >> >> I agree with Gary on this. >> >> Ralph >> >> On Apr 23, 2014, at 8:36 AM, Gary Gregory <[email protected]> wrote: >> >> On the call site side, it seems cleaner to catch an IAE and log the fact >> that some programming error took place. If I catch NPE, I cannot say that >> there is a bug or that someone is passing a known bad value, all I can say >> is that a null object is where the program does not allow it. >> >> We'll have to agree to disagree. >> >> Gary >> >> >> On Wed, Apr 23, 2014 at 10:49 AM, Matt Sicker <[email protected]> wrote: >> >>> I used to think like that, but that was due to the fact that not a >>> single NPE in the JDK includes an error message. Thus, NPE gets the >>> reputation of being a completely useless exception to find in your stack >>> track. In my opinion, IAE makes sense when the argument doesn't follow the >>> rules other than nullity. For example, I changed the isInstanceOf method to >>> not throw an exception if the marker doesn't exist, but if it were kept as >>> an exception, I'd use an IAE since it indicates the given marker is not a >>> valid argument. >>> >>> This is pretty consistent with the JDK with the added bonus of useful >>> exception messages. >>> >>> >>> On 23 April 2014 08:25, Gary Gregory <[email protected]> wrote: >>> >>>> I like IAE better than NPE because it shows log4j knows the argument is >>>> not allowed, null or otherwise, where NPE says (to me) 'some object was >>>> null and I blew up'. IAE is more of a 'this is not allowed by design, >>>> definitively not a bug in log4j'. >>>> >>>> Gary >>>> >>>> ---------- Forwarded message ---------- >>>> From: <[email protected]> >>>> Date: Wed, Apr 23, 2014 at 10:20 AM >>>> Subject: svn commit: r1589425 - >>>> /logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java >>>> To: [email protected] >>>> >>>> >>>> Author: mattsicker >>>> Date: Wed Apr 23 14:20:56 2014 >>>> New Revision: 1589425 >>>> >>>> URL: http://svn.apache.org/r1589425 >>>> Log: >>>> Update null handling. >>>> >>>> - Don't allow null Marker names. They won't work anyway due to the >>>> use of ConcurrentHashMap. >>>> - Update methods to throw NullPointerException instead of >>>> IllegalArgumentException to match updated javadocs. >>>> - Simplify equals and hashCode based on guaranteed non-nullity of >>>> marker name. >>>> >>>> Modified: >>>> >>>> logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java >>>> >>>> Modified: >>>> logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java >>>> URL: >>>> http://svn.apache.org/viewvc/logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java?rev=1589425&r1=1589424&r2=1589425&view=diff >>>> >>>> ============================================================================== >>>> --- >>>> logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java >>>> (original) >>>> +++ >>>> logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java >>>> Wed Apr 23 14:20:56 2014 >>>> @@ -36,6 +36,7 @@ public final class MarkerManager { >>>> * Retrieve a Marker or create a Marker that has no parent. >>>> * @param name The name of the Marker. >>>> * @return The Marker with the specified name. >>>> + * @throws NullPointerException if the argument is {@code null} >>>> */ >>>> public static Marker getMarker(final String name) { >>>> markerMap.putIfAbsent(name, new Log4jMarker(name)); >>>> @@ -48,6 +49,7 @@ public final class MarkerManager { >>>> * @param parent The name of the parent Marker. >>>> * @return The Marker with the specified name. >>>> * @throws IllegalArgumentException if the parent Marker does not >>>> exist. >>>> + * @throws NullPointerException if the argument is {@code null} >>>> * @deprecated Use the Marker add or set methods to add parent >>>> Markers. Will be removed by final GA release. >>>> */ >>>> @Deprecated >>>> @@ -66,6 +68,7 @@ public final class MarkerManager { >>>> * @param name The name of the Marker. >>>> * @param parent The parent Marker. >>>> * @return The Marker with the specified name. >>>> + * @throws NullPointerException if the argument is {@code null} >>>> * @deprecated Use the Marker add or set methods to add parent >>>> Markers. Will be removed by final GA release. >>>> */ >>>> @Deprecated >>>> @@ -83,15 +86,27 @@ public final class MarkerManager { >>>> private final String name; >>>> private volatile Marker[] parents; >>>> >>>> + /** >>>> + * Constructs a new Marker. >>>> + * @param name the name of the Marker. >>>> + * @throws NullPointerException if the argument is {@code null} >>>> + */ >>>> public Log4jMarker(final String name) { >>>> + if (name == null) { >>>> + // we can't store null references in a >>>> ConcurrentHashMap as it is, not to mention that a null Marker >>>> + // name seems rather pointless. To get an "anonymous" >>>> Marker, just use an empty string. >>>> + throw new NullPointerException("Marker name cannot be >>>> null."); >>>> + } >>>> this.name = name; >>>> this.parents = null; >>>> } >>>> >>>> + // TODO: use java.util.concurrent >>>> + >>>> @Override >>>> public synchronized Marker add(final Marker parent) { >>>> if (parent == null) { >>>> - throw new IllegalArgumentException("A parent marker >>>> must be specified"); >>>> + throw new NullPointerException("A parent marker must >>>> be specified"); >>>> } >>>> // It is not strictly necessary to copy the variable here >>>> but it should perform better than >>>> // Accessing a volatile variable multiple times. >>>> @@ -100,7 +115,6 @@ public final class MarkerManager { >>>> if (localParents != null && (contains(parent, >>>> localParents) || parent.isInstanceOf(this))) { >>>> return this; >>>> } >>>> - // FIXME: should we really be only increasing the array >>>> size by 1 each time? why not something like log(n)? >>>> final int size = localParents == null ? 1 : >>>> localParents.length + 1; >>>> final Marker[] markers = new Marker[size]; >>>> if (localParents != null) { >>>> @@ -113,10 +127,12 @@ public final class MarkerManager { >>>> return this; >>>> } >>>> >>>> + // TODO: add(Marker parent, Marker... moreParents) >>>> + >>>> @Override >>>> public synchronized boolean remove(final Marker parent) { >>>> if (parent == null) { >>>> - throw new IllegalArgumentException("A parent marker >>>> must be specified"); >>>> + throw new NullPointerException("A parent marker must >>>> be specified"); >>>> } >>>> final Marker[] localParents = this.parents; >>>> if (localParents == null) { >>>> @@ -184,7 +200,7 @@ public final class MarkerManager { >>>> @Override >>>> public boolean isInstanceOf(final Marker marker) { >>>> if (marker == null) { >>>> - throw new IllegalArgumentException("A marker parameter >>>> is required"); >>>> + throw new NullPointerException("A marker parameter is >>>> required"); >>>> } >>>> if (this == marker) { >>>> return true; >>>> @@ -213,8 +229,7 @@ public final class MarkerManager { >>>> @Override >>>> public boolean isInstanceOf(final String markerName) { >>>> if (markerName == null) { >>>> - // FIXME: and normally you'd throw an NPE here (ditto >>>> for other cases above) >>>> - throw new IllegalArgumentException("A marker name is >>>> required"); >>>> + throw new NullPointerException("A marker name is >>>> required"); >>>> } >>>> if (markerName.equals(this.getName())) { >>>> return true; >>>> @@ -222,8 +237,7 @@ public final class MarkerManager { >>>> // Use a real marker for child comparisons. It is faster >>>> than comparing the names. >>>> final Marker marker = markerMap.get(markerName); >>>> if (marker == null) { >>>> - // FIXME: shouldn't this just return false? >>>> - throw new IllegalArgumentException("No marker exists >>>> with the name " + markerName); >>>> + return false; >>>> } >>>> final Marker[] localParents = parents; >>>> if (localParents != null) { >>>> @@ -292,19 +306,13 @@ public final class MarkerManager { >>>> if (o == null || !(o instanceof Marker)) { >>>> return false; >>>> } >>>> - >>>> final Marker marker = (Marker) o; >>>> - >>>> - if (name != null ? !name.equals(marker.getName()) : >>>> marker.getName() != null) { >>>> - return false; >>>> - } >>>> - >>>> - return true; >>>> + return name.equals(marker.getName()); >>>> } >>>> >>>> @Override >>>> public int hashCode() { >>>> - return name != null ? name.hashCode() : 0; >>>> + return name.hashCode(); >>>> } >>>> >>>> @Override >>>> >>>> >>>> >>>> >>>> >>>> -- >>>> E-Mail: [email protected] | [email protected] >>>> Java Persistence with Hibernate, Second >>>> Edition<http://www.manning.com/bauer3/> >>>> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> >>>> Spring Batch in Action <http://www.manning.com/templier/> >>>> Blog: http://garygregory.wordpress.com >>>> Home: http://garygregory.com/ >>>> Tweet! http://twitter.com/GaryGregory >>>> >>> >>> >>> >>> -- >>> Matt Sicker <[email protected]> >>> >> >> >> >> -- >> E-Mail: [email protected] | [email protected] >> Java Persistence with Hibernate, Second >> Edition<http://www.manning.com/bauer3/> >> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> >> Spring Batch in Action <http://www.manning.com/templier/> >> Blog: http://garygregory.wordpress.com >> Home: http://garygregory.com/ >> Tweet! http://twitter.com/GaryGregory >> >> >> >> > > > -- > Matt Sicker <[email protected]> > -- Matt Sicker <[email protected]>
