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 >> 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 >
