On Tue, Dec 2, 2014 at 6:44 PM, Remko Popma <[email protected]> wrote:
> Not sure that I agree with this not being a functional change: The null
> check used to be on cause.getCauseProxy(), now it is on cause.
>
The old pattern was:
if (cause != null) {
foo(cause);
}
...
if (cause != null) {
foo(cause);
}
void foo(cause) {
...
}
Now we have:
foo(cause);
...
foo(cause);
void foo(cause) {
if (cause == null) {
return;
}
...
}
How is that not equivalent?
Gary
>
> About unit tests: I believe the problem scenario is when the Throwable
> class does not exist on the process that deserialized the ThrowableProxy.
> It is difficult to create a test for this. If I'm not mistaken though this
> is one of the main use cases for throwable proxy so we cannot ignore it.
>
>
> On Wednesday, December 3, 2014, Gary Gregory <[email protected]>
> wrote:
>
>> Yes: This is a change that refactors guard clauses, no behavioral change.
>> Tests pass.
>> No: I do not know if the specific case you mention is covered by the unit
>> tests or the code for that matter. The tests do pass though.
>> Thoughts?
>>
>> Gary
>>
>> On Tue, Dec 2, 2014 at 5:01 PM, Remko Popma <[email protected]>
>> wrote:
>>
>>> Are you sure this is correct?
>>> What about deserialized ThrowableProxies where the cause is null but the
>>> causeProxy is non-null?
>>>
>>> Sent from my iPhone
>>>
>>> > On 2014/12/03, at 3:10, [email protected] wrote:
>>> >
>>> > Repository: logging-log4j2
>>> > Updated Branches:
>>> > refs/heads/master ab58a6d69 -> df71c934d
>>> >
>>> >
>>> > Let the formatCause method have a null guard clause instead of each
>>> call
>>> > site guarding the call.
>>> >
>>> > Project: http://git-wip-us.apache.org/repos/asf/logging-log4j2/repo
>>> > Commit:
>>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/commit/df71c934
>>> > Tree:
>>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/tree/df71c934
>>> > Diff:
>>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/diff/df71c934
>>> >
>>> > Branch: refs/heads/master
>>> > Commit: df71c934d59570bbee10d8eb413d92eaa443bec4
>>> > Parents: ab58a6d
>>> > Author: Gary Gregory <[email protected]>
>>> > Authored: Tue Dec 2 13:10:10 2014 -0500
>>> > Committer: Gary Gregory <[email protected]>
>>> > Committed: Tue Dec 2 13:10:10 2014 -0500
>>> >
>>> > ----------------------------------------------------------------------
>>> > .../apache/logging/log4j/core/impl/ThrowableProxy.java | 11
>>> +++++------
>>> > 1 file changed, 5 insertions(+), 6 deletions(-)
>>> > ----------------------------------------------------------------------
>>> >
>>> >
>>> >
>>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/df71c934/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableProxy.java
>>> > ----------------------------------------------------------------------
>>> > diff --git
>>> a/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableProxy.java
>>> b/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableProxy.java
>>> > index 3aadf6c..23065ab 100644
>>> > ---
>>> a/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableProxy.java
>>> > +++
>>> b/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableProxy.java
>>> > @@ -183,12 +183,13 @@ public class ThrowableProxy implements
>>> Serializable {
>>> >
>>> > @SuppressWarnings("ThrowableResultOfMethodCallIgnored")
>>> > private void formatCause(final StringBuilder sb, final
>>> ThrowableProxy cause, final List<String> ignorePackages) {
>>> > + if (cause == null) {
>>> > + return;
>>> > + }
>>> > sb.append("Caused by: ").append(cause).append(EOL);
>>> > this.formatElements(sb, cause.commonElementCount,
>>> cause.getThrowable().getStackTrace(),
>>> > cause.extendedStackTrace, ignorePackages);
>>> > - if (cause.getCauseProxy() != null) {
>>> > - this.formatCause(sb, cause.causeProxy, ignorePackages);
>>> > - }
>>> > + this.formatCause(sb, cause.causeProxy, ignorePackages);
>>> > }
>>> >
>>> > private void formatElements(final StringBuilder sb, final int
>>> commonCount, final StackTraceElement[] causedTrace,
>>> > @@ -342,9 +343,7 @@ public class ThrowableProxy implements
>>> Serializable {
>>> > }
>>> > sb.append('\n');
>>> > this.formatElements(sb, 0, this.throwable.getStackTrace(),
>>> this.extendedStackTrace, ignorePackages);
>>> > - if (this.causeProxy != null) {
>>> > - this.formatCause(sb, this.causeProxy, ignorePackages);
>>> > - }
>>> > + this.formatCause(sb, this.causeProxy, ignorePackages);
>>> > return sb.toString();
>>> > }
>>> >
>>> >
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: [email protected]
>>> For additional commands, e-mail: [email protected]
>>>
>>>
>>
>>
>> --
>> E-Mail: [email protected] | [email protected]
>> Java Persistence with Hibernate, Second Edition
>> <http://www.manning.com/bauer3/>
>> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
>> Spring Batch in Action <http://www.manning.com/templier/>
>> Blog: http://garygregory.wordpress.com
>> Home: http://garygregory.com/
>> Tweet! http://twitter.com/GaryGregory
>>
>
--
E-Mail: [email protected] | [email protected]
Java Persistence with Hibernate, Second Edition
<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory
