You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@logging.apache.org by ma...@apache.org on 2014/04/23 16:20:56 UTC
svn commit: r1589425 -
/logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
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
Re: svn commit: r1589425 - /logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
Posted by Matt Sicker <bo...@gmail.com>.
Updated in 1589555.
On 23 April 2014 16:51, Matt Sicker <bo...@gmail.com> 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 <ra...@dslextreme.com> 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 <ra...@dslextreme.com>
>> wrote:
>>
>> I agree with Gary on this.
>>
>> Ralph
>>
>> On Apr 23, 2014, at 8:36 AM, Gary Gregory <ga...@gmail.com> 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 <bo...@gmail.com> 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 <ga...@gmail.com> 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: <ma...@apache.org>
>>>> 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: commits@logging.apache.org
>>>>
>>>>
>>>> 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: garydgregory@gmail.com | ggregory@apache.org
>>>> 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 <bo...@gmail.com>
>>>
>>
>>
>>
>> --
>> E-Mail: garydgregory@gmail.com | ggregory@apache.org
>> 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 <bo...@gmail.com>
>
--
Matt Sicker <bo...@gmail.com>
Re: svn commit: r1589425 - /logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
Posted by Matt Sicker <bo...@gmail.com>.
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 <ra...@dslextreme.com> 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 <ra...@dslextreme.com>
> wrote:
>
> I agree with Gary on this.
>
> Ralph
>
> On Apr 23, 2014, at 8:36 AM, Gary Gregory <ga...@gmail.com> 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 <bo...@gmail.com> 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 <ga...@gmail.com> 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: <ma...@apache.org>
>>> 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: commits@logging.apache.org
>>>
>>>
>>> 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: garydgregory@gmail.com | ggregory@apache.org
>>> 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 <bo...@gmail.com>
>>
>
>
>
> --
> E-Mail: garydgregory@gmail.com | ggregory@apache.org
> 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 <bo...@gmail.com>
Re: svn commit: r1589425 - /logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
Posted by Ralph Goers <ra...@dslextreme.com>.
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 <ra...@dslextreme.com> wrote:
> I agree with Gary on this.
>
> Ralph
>
> On Apr 23, 2014, at 8:36 AM, Gary Gregory <ga...@gmail.com> 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 <bo...@gmail.com> 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 <ga...@gmail.com> 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: <ma...@apache.org>
>> 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: commits@logging.apache.org
>>
>>
>> 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: garydgregory@gmail.com | ggregory@apache.org
>> 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 <bo...@gmail.com>
>>
>>
>>
>> --
>> E-Mail: garydgregory@gmail.com | ggregory@apache.org
>> 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
>
Re: svn commit: r1589425 - /logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
Posted by Ralph Goers <ra...@dslextreme.com>.
I agree with Gary on this.
Ralph
On Apr 23, 2014, at 8:36 AM, Gary Gregory <ga...@gmail.com> 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 <bo...@gmail.com> 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 <ga...@gmail.com> 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: <ma...@apache.org>
> 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: commits@logging.apache.org
>
>
> 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: garydgregory@gmail.com | ggregory@apache.org
> 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 <bo...@gmail.com>
>
>
>
> --
> E-Mail: garydgregory@gmail.com | ggregory@apache.org
> 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
Re: svn commit: r1589425 - /logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
Posted by Gary Gregory <ga...@gmail.com>.
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 <bo...@gmail.com> 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 <ga...@gmail.com> 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: <ma...@apache.org>
>> 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: commits@logging.apache.org
>>
>>
>> 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: garydgregory@gmail.com | ggregory@apache.org
>> 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 <bo...@gmail.com>
>
--
E-Mail: garydgregory@gmail.com | ggregory@apache.org
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
Re: svn commit: r1589425 - /logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
Posted by Matt Sicker <bo...@gmail.com>.
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 <ga...@gmail.com> 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: <ma...@apache.org>
> 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: commits@logging.apache.org
>
>
> 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: garydgregory@gmail.com | ggregory@apache.org
> 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 <bo...@gmail.com>
Fwd: svn commit: r1589425 - /logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
Posted by Gary Gregory <ga...@gmail.com>.
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: <ma...@apache.org>
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: commits@logging.apache.org
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: garydgregory@gmail.com | ggregory@apache.org
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