On Tue, Dec 2, 2014 at 9:49 PM, Ralph Goers <[email protected]> wrote:
> Please see http://logging.apache.org/log4j/2.x/guidelines.html#teamwork item > 4. > > “Fixing” things that only you have identified as a problem should have > been preceded by a discussion. > Wow, well, that seems like a pretty small change to be worth a ML discussion. I'll have to think twice before touching code if we are going to do RTC for stuff like this. I thought we developed in CTR mode. Are you saying we should do RTC? Gary > > Ralph > > > On Dec 2, 2014, at 6:54 PM, Gary Gregory <[email protected]> wrote: > > On Tue, Dec 2, 2014 at 6:37 PM, Ralph Goers <[email protected]> > wrote: > >> Why did you make the change? >> > > Because I do not like code duplication and we had null checks before > calling a method. It looks less cluttered now IMO. > > >> Was something broken? >> > > Not for the normal definition of "broken". But we don't have to only make > changes to "fix" something... > > Gary > > >> >> Ralph >> >> On Dec 2, 2014, at 3:08 PM, 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] >> <[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] > <[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
