I've just created a webrev with all the changes we've discussed so far. Plus some more I've spotted while looking into the code. Please note, this webrev is not incremental. It grabs all the changes between the original patch and the latest discussed:
http://cr.openjdk.java.net/~prappo/8048267/webrev.02 Andrej, you can verify that all your changes are included as well as Paul's. As a workaround for this particular review thread you can download changeset files from consecutive webrevs and just diff them: http://cr.openjdk.java.net/~prappo/8048267/webrev.$(i)/jdk.changeset http://cr.openjdk.java.net/~prappo/8048267/webrev.$(i+1)/jdk.changeset In addition to these I also found some more. It turns out such simple changes can lead to infinite recursion calls. Compare these: (line 622): http://cr.openjdk.java.net/~prappo/8048267/webrev.01/src/share/classes/com/sun/tools/hat/internal/parser/HprofReader.java.sdiff.html http://cr.openjdk.java.net/~prappo/8048267/webrev.02/src/share/classes/com/sun/tools/hat/internal/parser/HprofReader.java.sdiff.html (lines 384, 508): http://cr.openjdk.java.net/~prappo/8048267/webrev.01/src/share/classes/java/awt/image/renderable/ParameterBlock.java.sdiff.html http://cr.openjdk.java.net/~prappo/8048267/webrev.02/src/share/classes/java/awt/image/renderable/ParameterBlock.java.sdiff.html 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 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. 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. -Pavel On 27 Jun 2014, at 11:52, Pavel Rappo <pavel.ra...@oracle.com> wrote: > Hi Andrej, > > They are not identical. Maybe it's just hard to spot it as it's not an > incremental webrev. Have a look at > > (line 583): > > http://cr.openjdk.java.net/~prappo/8048267/webrev.00/src/share/classes/javax/management/modelmbean/RequiredModelMBean.java.sdiff.html > http://cr.openjdk.java.net/~prappo/8048267/webrev.01/src/share/classes/javax/management/modelmbean/RequiredModelMBean.java.sdiff.html > > (line 90): > > http://cr.openjdk.java.net/~prappo/8048267/webrev.00/src/share/classes/com/sun/security/auth/UnixNumericUserPrincipal.java.sdiff.html > http://cr.openjdk.java.net/~prappo/8048267/webrev.01/src/share/classes/com/sun/security/auth/UnixNumericUserPrincipal.java.sdiff.html > > Though I've missed one you suggested in RequiredModelMBean.java:547 > Anyway, I'll try to incorporate everything you've spotted so far. > > Thanks, > -Pavel > > On 27 Jun 2014, at 11:36, Andrej Golovnin <andrej.golov...@gmail.com> wrote: > >> Hi Pavel, >> >> the both web revs looks identical to me. Here is what I have found so far in >> the webrev.01: >> >> in src/share/classes/com/sun/security/auth/SolarisNumericGroupPrincipal.java: >> >> @@ -108,11 +108,11 @@ >> * @param primaryGroup true if the specified GID represents the >> * primary group to which this user belongs. >> * >> */ >> public SolarisNumericGroupPrincipal(long name, boolean primaryGroup) { >> - this.name = (new Long(name)).toString(); >> + this.name = Long.valueOf(name).toString(); >> this.primaryGroup = primaryGroup; >> } >> >> It is better to use Long.toString(long): >> >> + this.name = Long.toString(name); >> >> This also applies to: >> >> src/share/classes/com/sun/security/auth/SolarisNumericUserPrincipal.java >> src/share/classes/com/sun/security/auth/UnixNumericGroupPrincipal.java >> src/share/classes/com/sun/security/auth/UnixNumericUserPrincipal.java >> >> >> @@ -94,11 +94,11 @@ >> * >> * @param name the user identification number (UID) for this user >> * represented as a long. >> */ >> public SolarisNumericUserPrincipal(long name) { >> - this.name = (new Long(name)).toString(); >> + this.name = Long.valueOf(name).toString(); >> } >> >> >> In src/share/classes/javax/management/modelmbean/RequiredModelMBean.java: >> >> @@ -542,11 +542,11 @@ >> RequiredModelMBean.class.getName(), >> mth,"currencyTimeLimit: " + expTime); >> } >> >> // convert seconds to milliseconds for time comparison >> - currencyPeriod = ((new Long(expTime)).longValue()) * 1000; >> + currencyPeriod = ((Long.valueOf(expTime)).longValue()) * 1000; >> >> Please use Long.parseLong(String), e.g.: >> >> + currencyPeriod = Long.parseLong(expTime) * 1000; >> >> And here please use Long.parseLong8String) too: >> >> @@ -578,11 +578,11 @@ >> } >> >> if (tStamp == null) >> tStamp = "0"; >> >> - long lastTime = (new Long(tStamp)).longValue(); >> + long lastTime = (Long.valueOf(tStamp)).longValue(); >> >> e.g.: >> >> + long lastTime = Long.parseLong(tStamp); >> >> >> In src/share/classes/sun/security/jgss/wrapper/GSSNameElement.java >> >> @@ -201,11 +201,11 @@ >> return false; >> } >> } >> >> public int hashCode() { >> - return new Long(pName).hashCode(); >> + return Long.valueOf(pName).hashCode(); >> } >> >> The method Long.hashCode(long) (added in JDK 8) should be used to calculate >> the hash for a long value, e.g.: >> >> + return Long.hashCode(pName); >> >> Best regards, >> Andrej Golovnin >> >> >> >> >> On Fri, Jun 27, 2014 at 12:00 PM, Pavel Rappo <pavel.ra...@oracle.com> wrote: >> I created an issue to track the progress and also made 2 webrevs. One for >> the original patch and one for the changes that have been suggested earlier >> in this thread by Paul and Andrej. Here we go: >> >> https://bugs.openjdk.java.net/browse/JDK-8048267 >> >> http://cr.openjdk.java.net/~prappo/8048267/webrev.00 >> http://cr.openjdk.java.net/~prappo/8048267/webrev.01 >> >> -Pavel >> >> On 26 Jun 2014, at 10:58, Chris Hegarty <chris.hega...@oracle.com> wrote: >> >>> Otavio, >>> >>> I skimmed over the patches and they look ok to me. I think they would be >>> suitable for inclusion in jdk9/dev. >>> >>> -Chris. >>> >>> P.S. I do not currently have time to sponsor this, but I cc’ed Pavel who >>> may be able to help get his in. >>> >>> On 14 Jun 2014, at 14:46, Otávio Gonçalves de Santana <otavioj...@java.net> >>> wrote: >>> >>>> Reason: The Long class has cache and using it, will save memory and will >>>> faster than using create new instance of Long. >>>> >>>> webrev: >>>> https://dl.dropboxusercontent.com/u/16109193/open_jdk/long_value_of.zip >>>> >>>> similar: https://bugs.openjdk.java.net/browse/JDK-8044461 >>>> -- >>>> 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 >>>> <sun_tools.diff><sun_security.diff><sun_nio.diff><sun_management.diff><sun_jvmstat.diff><javax_swing.diff><javax-management.diff><java_text.diff><java_awt_image.diff><internal_org_objectweb.diff><com_sun_tools.diff><com_sun_security.diff><com_sun_jmx_snmp.diff><com_sun_jdni_ldap.diff><com_sun_imageio.diff> >>> >> >> >
