You are viewing a plain text version of this content. The canonical link for it is here.
Posted to log4j-dev@logging.apache.org by Gary Gregory <ga...@gmail.com> on 2016/02/29 03:05:45 UTC
Fwd: [2/2] logging-log4j2 git commit: Extract a requireNonNull method.
How about using java.lang.Objects?
Gary
---------- Forwarded message ----------
From: <ma...@apache.org>
Date: Feb 28, 2016 5:49 PM
Subject: [2/2] logging-log4j2 git commit: Extract a requireNonNull method.
To: <co...@logging.apache.org>
Cc:
Extract a requireNonNull method.
In the past, these method calls could have simply used
Objects.requireNonNull, but a later revision switched them to use
IllegalArgumentException.
Project: http://git-wip-us.apache.org/repos/asf/logging-log4j2/repo
Commit:
http://git-wip-us.apache.org/repos/asf/logging-log4j2/commit/9d63b0d1
Tree: http://git-wip-us.apache.org/repos/asf/logging-log4j2/tree/9d63b0d1
Diff: http://git-wip-us.apache.org/repos/asf/logging-log4j2/diff/9d63b0d1
Branch: refs/heads/master
Commit: 9d63b0d11fe3d24b0b3d670ab7f3b369c0f6e1dd
Parents: 96dff0c
Author: Matt Sicker <bo...@gmail.com>
Authored: Sun Feb 28 19:49:43 2016 -0600
Committer: Matt Sicker <bo...@gmail.com>
Committed: Sun Feb 28 19:49:43 2016 -0600
----------------------------------------------------------------------
.../org/apache/logging/log4j/MarkerManager.java | 40 +++++++++-----------
1 file changed, 18 insertions(+), 22 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/9d63b0d1/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
----------------------------------------------------------------------
diff --git
a/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
b/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
index d2b3d19..d57c483 100644
--- a/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
+++ b/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
@@ -42,7 +42,7 @@ public final class MarkerManager {
/**
* Tests existence of the given marker.
- *
+ *
* @param key the marker name
* @return true if the marker exists.
* @since 2.4
@@ -53,7 +53,7 @@ public final class MarkerManager {
/**
* Retrieves 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 IllegalArgumentException if the argument is {@code null}
@@ -65,7 +65,7 @@ public final class MarkerManager {
/**
* Retrieves or creates a Marker with the specified parent. The parent
must have been previously created.
- *
+ *
* @param name The name of the Marker.
* @param parent The name of the parent Marker.
* @return The Marker with the specified name.
@@ -85,7 +85,7 @@ public final class MarkerManager {
/**
* Retrieves or creates a Marker with the specified parent.
- *
+ *
* @param name The name of the Marker.
* @param parent The parent Marker.
* @return The Marker with the specified name.
@@ -128,16 +128,14 @@ public final class MarkerManager {
/**
* Constructs a new Marker.
- *
+ *
* @param name the name of the Marker.
* @throws IllegalArgumentException 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 IllegalArgumentException("Marker name cannot be
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.
+ requireNonNull(name, "Marker name cannot be null.");
this.name = name;
this.parents = null;
}
@@ -146,9 +144,7 @@ public final class MarkerManager {
@Override
public synchronized Marker addParents(final Marker...
parentMarkers) {
- if (parentMarkers == null) {
- throw new IllegalArgumentException("A parent marker must
be specified");
- }
+ requireNonNull(parentMarkers, "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.
final Marker[] localParents = this.parents;
@@ -184,9 +180,7 @@ public final class MarkerManager {
@Override
public synchronized boolean remove(final Marker parent) {
- if (parent == null) {
- throw new IllegalArgumentException("A parent marker must
be specified");
- }
+ requireNonNull(parent, "A parent marker must be specified");
final Marker[] localParents = this.parents;
if (localParents == null) {
return false;
@@ -249,9 +243,7 @@ public final class MarkerManager {
@Override
@PerformanceSensitive({"allocation", "unrolled"})
public boolean isInstanceOf(final Marker marker) {
- if (marker == null) {
- throw new IllegalArgumentException("A marker parameter is
required");
- }
+ requireNonNull(marker, "A marker parameter is required");
if (this == marker) {
return true;
}
@@ -279,9 +271,7 @@ public final class MarkerManager {
@Override
@PerformanceSensitive({"allocation", "unrolled"})
public boolean isInstanceOf(final String markerName) {
- if (markerName == null) {
- throw new IllegalArgumentException("A marker name is
required");
- }
+ requireNonNull(markerName, "A marker name is required");
if (markerName.equals(this.getName())) {
return true;
}
@@ -402,4 +392,10 @@ public final class MarkerManager {
}
}
+ // this method wouldn't be necessary if Marker methods threw an NPE
instead of an IAE for null values ;)
+ private static void requireNonNull(final Object obj, final String
message) {
+ if (obj == null) {
+ throw new IllegalArgumentException(message);
+ }
+ }
}
Re: [2/2] logging-log4j2 git commit: Extract a requireNonNull method.
Posted by Gary Gregory <ga...@gmail.com>.
Crud, I guess that's a no go.
Gary
On Feb 28, 2016 6:13 PM, "Matt Sicker" <bo...@gmail.com> wrote:
> That depends if changing from IllegalArgumentException to
> NullPointerException in the Marker interface is acceptable.
>
> On 28 February 2016 at 20:05, Gary Gregory <ga...@gmail.com> wrote:
>
>> How about using java.lang.Objects?
>>
>> Gary
>> ---------- Forwarded message ----------
>> From: <ma...@apache.org>
>> Date: Feb 28, 2016 5:49 PM
>> Subject: [2/2] logging-log4j2 git commit: Extract a requireNonNull method.
>> To: <co...@logging.apache.org>
>> Cc:
>>
>> Extract a requireNonNull method.
>>
>> In the past, these method calls could have simply used
>> Objects.requireNonNull, but a later revision switched them to use
>> IllegalArgumentException.
>>
>>
>> Project: http://git-wip-us.apache.org/repos/asf/logging-log4j2/repo
>> Commit:
>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/commit/9d63b0d1
>> Tree: http://git-wip-us.apache.org/repos/asf/logging-log4j2/tree/9d63b0d1
>> Diff: http://git-wip-us.apache.org/repos/asf/logging-log4j2/diff/9d63b0d1
>>
>> Branch: refs/heads/master
>> Commit: 9d63b0d11fe3d24b0b3d670ab7f3b369c0f6e1dd
>> Parents: 96dff0c
>> Author: Matt Sicker <bo...@gmail.com>
>> Authored: Sun Feb 28 19:49:43 2016 -0600
>> Committer: Matt Sicker <bo...@gmail.com>
>> Committed: Sun Feb 28 19:49:43 2016 -0600
>>
>> ----------------------------------------------------------------------
>> .../org/apache/logging/log4j/MarkerManager.java | 40 +++++++++-----------
>> 1 file changed, 18 insertions(+), 22 deletions(-)
>> ----------------------------------------------------------------------
>>
>>
>>
>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/9d63b0d1/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
>> ----------------------------------------------------------------------
>> diff --git
>> a/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
>> b/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
>> index d2b3d19..d57c483 100644
>> --- a/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
>> +++ b/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
>> @@ -42,7 +42,7 @@ public final class MarkerManager {
>>
>> /**
>> * Tests existence of the given marker.
>> - *
>> + *
>> * @param key the marker name
>> * @return true if the marker exists.
>> * @since 2.4
>> @@ -53,7 +53,7 @@ public final class MarkerManager {
>>
>> /**
>> * Retrieves 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 IllegalArgumentException if the argument is {@code null}
>> @@ -65,7 +65,7 @@ public final class MarkerManager {
>>
>> /**
>> * Retrieves or creates a Marker with the specified parent. The
>> parent must have been previously created.
>> - *
>> + *
>> * @param name The name of the Marker.
>> * @param parent The name of the parent Marker.
>> * @return The Marker with the specified name.
>> @@ -85,7 +85,7 @@ public final class MarkerManager {
>>
>> /**
>> * Retrieves or creates a Marker with the specified parent.
>> - *
>> + *
>> * @param name The name of the Marker.
>> * @param parent The parent Marker.
>> * @return The Marker with the specified name.
>> @@ -128,16 +128,14 @@ public final class MarkerManager {
>>
>> /**
>> * Constructs a new Marker.
>> - *
>> + *
>> * @param name the name of the Marker.
>> * @throws IllegalArgumentException 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 IllegalArgumentException("Marker name cannot
>> be 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.
>> + requireNonNull(name, "Marker name cannot be null.");
>> this.name = name;
>> this.parents = null;
>> }
>> @@ -146,9 +144,7 @@ public final class MarkerManager {
>>
>> @Override
>> public synchronized Marker addParents(final Marker...
>> parentMarkers) {
>> - if (parentMarkers == null) {
>> - throw new IllegalArgumentException("A parent marker must
>> be specified");
>> - }
>> + requireNonNull(parentMarkers, "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.
>> final Marker[] localParents = this.parents;
>> @@ -184,9 +180,7 @@ public final class MarkerManager {
>>
>> @Override
>> public synchronized boolean remove(final Marker parent) {
>> - if (parent == null) {
>> - throw new IllegalArgumentException("A parent marker must
>> be specified");
>> - }
>> + requireNonNull(parent, "A parent marker must be specified");
>> final Marker[] localParents = this.parents;
>> if (localParents == null) {
>> return false;
>> @@ -249,9 +243,7 @@ public final class MarkerManager {
>> @Override
>> @PerformanceSensitive({"allocation", "unrolled"})
>> public boolean isInstanceOf(final Marker marker) {
>> - if (marker == null) {
>> - throw new IllegalArgumentException("A marker parameter
>> is required");
>> - }
>> + requireNonNull(marker, "A marker parameter is required");
>> if (this == marker) {
>> return true;
>> }
>> @@ -279,9 +271,7 @@ public final class MarkerManager {
>> @Override
>> @PerformanceSensitive({"allocation", "unrolled"})
>> public boolean isInstanceOf(final String markerName) {
>> - if (markerName == null) {
>> - throw new IllegalArgumentException("A marker name is
>> required");
>> - }
>> + requireNonNull(markerName, "A marker name is required");
>> if (markerName.equals(this.getName())) {
>> return true;
>> }
>> @@ -402,4 +392,10 @@ public final class MarkerManager {
>> }
>> }
>>
>> + // this method wouldn't be necessary if Marker methods threw an NPE
>> instead of an IAE for null values ;)
>> + private static void requireNonNull(final Object obj, final String
>> message) {
>> + if (obj == null) {
>> + throw new IllegalArgumentException(message);
>> + }
>> + }
>> }
>>
>>
>
>
> --
> Matt Sicker <bo...@gmail.com>
>
Re: [2/2] logging-log4j2 git commit: Extract a requireNonNull method.
Posted by Matt Sicker <bo...@gmail.com>.
That depends if changing from IllegalArgumentException to
NullPointerException in the Marker interface is acceptable.
On 28 February 2016 at 20:05, Gary Gregory <ga...@gmail.com> wrote:
> How about using java.lang.Objects?
>
> Gary
> ---------- Forwarded message ----------
> From: <ma...@apache.org>
> Date: Feb 28, 2016 5:49 PM
> Subject: [2/2] logging-log4j2 git commit: Extract a requireNonNull method.
> To: <co...@logging.apache.org>
> Cc:
>
> Extract a requireNonNull method.
>
> In the past, these method calls could have simply used
> Objects.requireNonNull, but a later revision switched them to use
> IllegalArgumentException.
>
>
> Project: http://git-wip-us.apache.org/repos/asf/logging-log4j2/repo
> Commit:
> http://git-wip-us.apache.org/repos/asf/logging-log4j2/commit/9d63b0d1
> Tree: http://git-wip-us.apache.org/repos/asf/logging-log4j2/tree/9d63b0d1
> Diff: http://git-wip-us.apache.org/repos/asf/logging-log4j2/diff/9d63b0d1
>
> Branch: refs/heads/master
> Commit: 9d63b0d11fe3d24b0b3d670ab7f3b369c0f6e1dd
> Parents: 96dff0c
> Author: Matt Sicker <bo...@gmail.com>
> Authored: Sun Feb 28 19:49:43 2016 -0600
> Committer: Matt Sicker <bo...@gmail.com>
> Committed: Sun Feb 28 19:49:43 2016 -0600
>
> ----------------------------------------------------------------------
> .../org/apache/logging/log4j/MarkerManager.java | 40 +++++++++-----------
> 1 file changed, 18 insertions(+), 22 deletions(-)
> ----------------------------------------------------------------------
>
>
>
> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/9d63b0d1/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
> ----------------------------------------------------------------------
> diff --git
> a/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
> b/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
> index d2b3d19..d57c483 100644
> --- a/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
> +++ b/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
> @@ -42,7 +42,7 @@ public final class MarkerManager {
>
> /**
> * Tests existence of the given marker.
> - *
> + *
> * @param key the marker name
> * @return true if the marker exists.
> * @since 2.4
> @@ -53,7 +53,7 @@ public final class MarkerManager {
>
> /**
> * Retrieves 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 IllegalArgumentException if the argument is {@code null}
> @@ -65,7 +65,7 @@ public final class MarkerManager {
>
> /**
> * Retrieves or creates a Marker with the specified parent. The
> parent must have been previously created.
> - *
> + *
> * @param name The name of the Marker.
> * @param parent The name of the parent Marker.
> * @return The Marker with the specified name.
> @@ -85,7 +85,7 @@ public final class MarkerManager {
>
> /**
> * Retrieves or creates a Marker with the specified parent.
> - *
> + *
> * @param name The name of the Marker.
> * @param parent The parent Marker.
> * @return The Marker with the specified name.
> @@ -128,16 +128,14 @@ public final class MarkerManager {
>
> /**
> * Constructs a new Marker.
> - *
> + *
> * @param name the name of the Marker.
> * @throws IllegalArgumentException 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 IllegalArgumentException("Marker name cannot be
> 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.
> + requireNonNull(name, "Marker name cannot be null.");
> this.name = name;
> this.parents = null;
> }
> @@ -146,9 +144,7 @@ public final class MarkerManager {
>
> @Override
> public synchronized Marker addParents(final Marker...
> parentMarkers) {
> - if (parentMarkers == null) {
> - throw new IllegalArgumentException("A parent marker must
> be specified");
> - }
> + requireNonNull(parentMarkers, "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.
> final Marker[] localParents = this.parents;
> @@ -184,9 +180,7 @@ public final class MarkerManager {
>
> @Override
> public synchronized boolean remove(final Marker parent) {
> - if (parent == null) {
> - throw new IllegalArgumentException("A parent marker must
> be specified");
> - }
> + requireNonNull(parent, "A parent marker must be specified");
> final Marker[] localParents = this.parents;
> if (localParents == null) {
> return false;
> @@ -249,9 +243,7 @@ public final class MarkerManager {
> @Override
> @PerformanceSensitive({"allocation", "unrolled"})
> public boolean isInstanceOf(final Marker marker) {
> - if (marker == null) {
> - throw new IllegalArgumentException("A marker parameter is
> required");
> - }
> + requireNonNull(marker, "A marker parameter is required");
> if (this == marker) {
> return true;
> }
> @@ -279,9 +271,7 @@ public final class MarkerManager {
> @Override
> @PerformanceSensitive({"allocation", "unrolled"})
> public boolean isInstanceOf(final String markerName) {
> - if (markerName == null) {
> - throw new IllegalArgumentException("A marker name is
> required");
> - }
> + requireNonNull(markerName, "A marker name is required");
> if (markerName.equals(this.getName())) {
> return true;
> }
> @@ -402,4 +392,10 @@ public final class MarkerManager {
> }
> }
>
> + // this method wouldn't be necessary if Marker methods threw an NPE
> instead of an IAE for null values ;)
> + private static void requireNonNull(final Object obj, final String
> message) {
> + if (obj == null) {
> + throw new IllegalArgumentException(message);
> + }
> + }
> }
>
>
--
Matt Sicker <bo...@gmail.com>