|
The patch supplied by Dave was reviewed/approved/applied by myself. It was applied with one exception. The change to ElemNumber was not applied.
I was not convinced that these were the same for a given char, ch: > String.valueOf(ch) > (new Character(ch)).toString(); Perhaps they result in the same String, or perhaps in the given context for the limited range of ch values that would be used they are the same, but I was not convinced. Also, I checked if this code path was ever taken for the "build smoketest" or "build alltest.conf" test buckets. The code path was not exercised, so it also lowered my confidence. (new Character(ch)).toString() and String.valueOf(ch) both produce one character String, only first one creates unnecessary Character object, char[] and calls native method System.arraycopy().
I used IntelliJ inspection to scan code for other String silliness, apply patch as you see fit. It's pointless to createStringBuffers and then concatenate strings by + (compiler will create another StringBuffer/StringBuilder) This issue is no longer relevant for 2.7.1, it is fixed in 2.7.1.
Changing the affected version to 2.7. Would the originator of this issue please verify that this issue is fixed in the 2.7.1 release, by adding a comment to this issue, so that we can close this issue.
A lack of response by February 1, 2008 will be taken as consent that we can close this resolved issue. Regards, Brian Minchau Patch applied correctly (with noted missing String.valueOf(ch) change)
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Improvement
Closed
Trivial
[PATCH] cleaup some String usage sillyness
-----------------------------------------------
Most are trivial .toString() methods called on a String, object, so the call is useless, and could even
create an additional String for no reason at all. I'm fine with these.
------------------------------------------------
OpMap has this change
new Integer(stepType).toString()
-------------> String.valueOf(stepType)
where stepType is an int. I looked up the Javadoc on String.valueOf(int) at
http://java.sun.com/j2se/1.4.2/docs/api/java/lang/String.html#valueOf(int)
and it said this:
"The representation is exactly the one returned by the Integer.toString method of one argument. "
so I'm OK with that one.
------------------------------------------------
The only change in the whole patch that has me concerned is the one in ElemNumber
(new Character(table.getChar((int)val - 1))).toString();
-----------> String.valueOf(table.getChar((int)val - 1));
table.getChar(int index) returns a char. I'm not sure that for a given char, ch, that
String.valueOf(ch)
is the same as
(new Character(ch)).toString();
The javadoc at:
http://java.sun.com/j2se/1.4.2/docs/api/java/lang/String.html#valueOf(char)
says this:
>> Returns the string representation of the char argument.
The javadoc for Charcter.toString() at:
http://java.sun.com/j2se/1.4.2/docs/api/java/lang/Character.html#toString()
says this:
>> Returns a String object representing this Character's value.
>>The result is a string of length 1 whose sole component is the primitive
>> char value represented by this Character object.
So are these always the same? I'm worried about some subtle differences between char and Character.
Perhaps the current locale can leak in somehow? I don't know for sure, just have my worries about this one.
These are always the same, then I'm ready to commit the patch to the code base.
- Brian