Long valueOf instead of new Long
Pavel Rappo
pavel.rappo at oracle.com
Mon Jun 30 10:11:25 UTC 2014
I've updated the webrev. It includes all the changes we've discussed so far plus these:
http://cr.openjdk.java.net/~prappo/8048267/webrev.03/src/macosx/classes/sun/font/CStrike.java.sdiff.html
http://cr.openjdk.java.net/~prappo/8048267/webrev.03/src/share/classes/com/sun/tools/example/debug/tty/BreakpointSpec.java.sdiff.html
http://cr.openjdk.java.net/~prappo/8048267/webrev.03/src/share/classes/sun/reflect/UnsafeLongFieldAccessorImpl.java.sdiff.html
http://cr.openjdk.java.net/~prappo/8048267/webrev.03/src/share/classes/sun/reflect/UnsafeQualifiedLongFieldAccessorImpl.java.sdiff.html
http://cr.openjdk.java.net/~prappo/8048267/webrev.03/src/share/classes/sun/reflect/UnsafeQualifiedStaticLongFieldAccessorImpl.java.sdiff.html
http://cr.openjdk.java.net/~prappo/8048267/webrev.03/src/share/classes/sun/reflect/UnsafeStaticLongFieldAccessorImpl.java.sdiff.html
http://cr.openjdk.java.net/~prappo/8048267/webrev.03/src/solaris/classes/java/util/prefs/FileSystemPreferences.java.sdiff.html
http://cr.openjdk.java.net/~prappo/8048267/webrev.03/src/solaris/classes/sun/awt/X11/XFocusProxyWindow.java.sdiff.html
Andrej, about the 0L -> Long0 change. I think it's not necessary since this case is *maybe* different from others. There's even a comment on it:
// Should never happen... but stay safe all the same.
//
else return 0L;
Anyway at a runtime 0L and Long0 will be the same object. So don't worry about that.
I think we're almost ready to go.
Thanks,
-Pavel
On 28 Jun 2014, at 00:09, Otávio Gonçalves de Santana <otaviojava at java.net> wrote:
> I found more two unnecessary valueOf.
> About Andrej, is it not possible add two people in "Contributed-by:" tag?
>
> diff -r d02b062bc827 src/share/classes/com/sun/tools/example/debug/tty/Commands.java
> --- a/src/share/classes/com/sun/tools/example/debug/tty/Commands.java Fri Jun 13 11:21:30 2014 -0700
> +++ b/src/share/classes/com/sun/tools/example/debug/tty/Commands.java Fri Jun 27 20:06:28 2014 -0300
> @@ -935,7 +935,7 @@
> try {
> methodInfo = loc.sourceName() +
> MessageOutput.format("line number",
> - new Object [] {new Long(lineNumber)});
> + new Object [] {lineNumber});
> } catch (AbsentInformationException e) {
> methodInfo = MessageOutput.format("unknown");
> }
> @@ -946,7 +946,7 @@
> meth.declaringType().name(),
> meth.name(),
> methodInfo,
> - new Long(pc)});
> + pc});
> } else {
> MessageOutput.println("stack frame dump",
> new Object [] {new Integer(frameNumber + 1),
>
>
> On Fri, Jun 27, 2014 at 5:16 PM, Andrej Golovnin <andrej.golovnin at gmail.com> wrote:
> Hi Pavel,
>
> I'm not sure what the style guide for the source code says, but
> there is a space between the cast operator and the field name in
> src/share/classes/com/sun/jmx/snmp/daemon/SnmpAdaptorServer.java (line 881):
>
> @Override
> public Long getSnmpOutGenErrs() {
> - return new Long(snmpOutGenErrs);
> + return (long) snmpOutGenErrs;
>
> }
>
> In all other changes there is no space after the cast operator.
>
>
> > Also, I removed one useless creation of a Long object here:
> >
> > (line 191):
> >
> > http://cr.openjdk.java.net/~prappo/8048267/webrev.01/src/share/classes/sun/security/krb5/internal/KerberosTime.java.sdiff.html
> > http://cr.openjdk.java.net/~prappo/8048267/webrev.02/src/share/classes/sun/security/krb5/internal/KerberosTime.java.sdiff.html
>
> And I have found one more :-) in KerberosTime.java:
>
> 252 public int getSeconds() {
> 253 Long temp_long = kerberosTime / 1000L;
> 254 return temp_long.intValue();
> 255 }
>
> This can be changed to:
>
> 252 public int getSeconds() {
> 253 long temp_long = kerberosTime / 1000L;
> 254 return (int) temp_long;
> 255 }
>
>
> >
> > I wonder if we should leave a cast to int here:
> >
> > (line 383):
> >
> > http://cr.openjdk.java.net/~prappo/8048267/webrev.01/src/share/classes/sun/management/snmp/jvminstr/JvmMemoryImpl.java.sdiff.html
> > http://cr.openjdk.java.net/~prappo/8048267/webrev.02/src/share/classes/sun/management/snmp/jvminstr/JvmMemoryImpl.java.sdiff.html
> >
> > Well it's probably nothing to worry about, but strictly speaking this changes the behaviour. Before the change, long was truncated to fit int. And now it's not.
>
> I would not change the behavior now. I think it is better to file a new issue and change
> it in a separate commit. Having this change in a separate commit may simplify tracking
> this change back in case it would cause some problems (I don't believe it).
> And in the new issue you may provide better description for this change.
>
> And a minor comment for JvmMemoryImpl.java:
> Maybe it is better to use Long0 in the line 387. So the code of the method
> JvmMemoryImpl.getJvmMemoryPendingFinalCount() will look similar to the code
> of other methods in the JvmMemoryImpl class. They all use the Long0 constant
> to return 0L.
>
> In sun/management/snmp/jvminstr/JvmThreadingImpl.java in the line 313:
>
> 312 public Long getJvmThreadPeakCount() throws SnmpStatusException {
> 313 return (long)getThreadMXBean().getPeakThreadCount();
> 314 }
>
> There is one space too much between "return" and the cast operator. The additional space
> was in the original version too, but maybe we can clean up here the code a little bit.
>
> >
> > P.S. Andrej, it looks like you don't have an 'Author' status. Well, that's a pity. We could mention you in the 'Reviewed-by' line. Your contributions are really good.
>
> Thanks! But I don't really care about it as long as I can help to improve the overall code quality.
>
> Best regards,
> Andrej Golovnin
>
>
>
>
> --
> Atenciosamente.
>
> Otávio Gonçalves de Santana
>
> blog: http://otaviosantana.blogspot.com.br/
> twitter: http://twitter.com/otaviojava
> site: http://www.otaviojava.com.br
> (11) 98255-3513
>
More information about the core-libs-dev
mailing list