I agree. I made some changes clarifying that (a) this is a performance sensitive method, and (b) why the method size is important and (c) linked to the Jira containing the performance measurements.
Note that I don't intend to do this with all methods. :-) Only places where profiling shows that log4j is spending most of its time. However, for such methods it is good to have such marker comments to prevent them from being casually modified. On Fri, Aug 21, 2015 at 1:38 AM, Gary Gregory <garydgreg...@gmail.com> wrote: > I think we need better comments than: > > // 30 bytes > > Think of what someone checking out the code would think and then do to > modify the code. > > At the very minimum a URL to the Jira issue is required. > > Gary > ---------- Forwarded message ---------- > From: <rpo...@apache.org> > Date: Thu, Aug 20, 2015 at 9:13 AM > Subject: logging-log4j2 git commit: LOG4J2-1096 bugfix and additional unit > test for case where one or more arguments is null > To: comm...@logging.apache.org > > > Repository: logging-log4j2 > Updated Branches: > refs/heads/master 8124d9b0d -> 31b7e7be5 > > > LOG4J2-1096 bugfix and additional unit test for case where one or more > arguments is null > > Project: http://git-wip-us.apache.org/repos/asf/logging-log4j2/repo > Commit: > http://git-wip-us.apache.org/repos/asf/logging-log4j2/commit/31b7e7be > Tree: http://git-wip-us.apache.org/repos/asf/logging-log4j2/tree/31b7e7be > Diff: http://git-wip-us.apache.org/repos/asf/logging-log4j2/diff/31b7e7be > > Branch: refs/heads/master > Commit: 31b7e7be51b2509680fe6e01c1ede99426ee81a0 > Parents: 8124d9b > Author: rpopma <rpo...@apache.org> > Authored: Fri Aug 21 01:13:37 2015 +0900 > Committer: rpopma <rpo...@apache.org> > Committed: Fri Aug 21 01:13:37 2015 +0900 > > ---------------------------------------------------------------------- > .../logging/log4j/message/ParameterizedMessage.java | 10 +++++----- > .../logging/log4j/message/ParameterizedMessageTest.java | 8 ++++++++ > 2 files changed, 13 insertions(+), 5 deletions(-) > ---------------------------------------------------------------------- > > > > http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/31b7e7be/log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterizedMessage.java > ---------------------------------------------------------------------- > diff --git > a/log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterizedMessage.java > b/log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterizedMessage.java > index 1a9175b..e5891c4 100644 > --- > a/log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterizedMessage.java > +++ > b/log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterizedMessage.java > @@ -323,11 +323,11 @@ public class ParameterizedMessage implements Message > { > /** > * Returns the sum of the lengths of all Strings in the specified > array. > */ > - // 27 bytes > + // 30 bytes > private static int sumStringLengths(final String[] arguments) { > int result = 0; > for (int i = 0; i < arguments.length; i++) { > - result += arguments[i].length(); > + result += String.valueOf(arguments[i]).length(); > } > return result; > } > @@ -441,11 +441,11 @@ public class ParameterizedMessage implements Message > { > * Appends the argument at the specified argument index to the > specified result char array at the specified position > * and returns the resulting position. > */ > - // 27 bytes > + // 30 bytes > private static int writeArgAt0(final String[] arguments, final int > currentArgument, final char[] result, > final int pos) { > - final String arg = arguments[currentArgument]; > - final int argLen = arg.length(); > + final String arg = String.valueOf(arguments[currentArgument]); > + int argLen = arg.length(); > arg.getChars(0, argLen, result, pos); > return pos + argLen; > } > > > http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/31b7e7be/log4j-api/src/test/java/org/apache/logging/log4j/message/ParameterizedMessageTest.java > ---------------------------------------------------------------------- > diff --git > a/log4j-api/src/test/java/org/apache/logging/log4j/message/ParameterizedMessageTest.java > b/log4j-api/src/test/java/org/apache/logging/log4j/message/ParameterizedMessageTest.java > index 5ca48df..d4ba0de 100644 > --- > a/log4j-api/src/test/java/org/apache/logging/log4j/message/ParameterizedMessageTest.java > +++ > b/log4j-api/src/test/java/org/apache/logging/log4j/message/ParameterizedMessageTest.java > @@ -46,6 +46,14 @@ public class ParameterizedMessageTest { > } > > @Test > + public void testFormatNullArgs() { > + final String testMsg = "Test message {} {} {} {} {} {}"; > + final String[] args = { "a", null, "c", null, null, null }; > + final String result = > ParameterizedMessage.formatStringArgs(testMsg, args); > + assertEquals("Test message a null c null null null", result); > + } > + > + @Test > public void testFormatStringArgsIgnoresSuperfluousArgs() { > final String testMsg = "Test message {}{} {}"; > final String[] args = { "a", "b", "c", "unnecessary", > "superfluous" }; > > > > > -- > E-Mail: garydgreg...@gmail.com | ggreg...@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 >
