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 > JUnit in Action, Second Edition > Spring Batch in Action > 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 > JUnit in Action, Second Edition > Spring Batch in Action > Blog: http://garygregory.wordpress.com > Home: http://garygregory.com/ > Tweet! http://twitter.com/GaryGregory
