Joel, the change looks good.
/R On Mar 26, 2013, at 11:32 AM, Joel Borggrén-Franck wrote: > Hi, > > Also including build-dev since this touches some makefiles. > > Thanks Dan! see inline, also new webrev: > > http://cr.openjdk.java.net/~jfranck/8009382/hotspot/webrev.02/ > > On 03/25/2013 04:17 PM, Daniel D. Daugherty wrote: >> On 3/22/13 8:16 AM, Joel Borggrén-Franck wrote: > >> make/bsd/makefiles/mapfile-vers-debug >> make/linux/makefiles/mapfile-vers-debug >> make/linux/makefiles/mapfile-vers-product >> make/solaris/makefiles/mapfile-vers >> It looks like the other entries are in alpha order so these >> two new entries should also be added in alpha order. >> > > Fixed. I also noticed I forgot to update one of the bsd makefiles. > >> src/share/vm/prims/jvm.cpp >> line 1510: return NULL; >> nit: indent should be two spaces >> >> line 1529: assert(false, "cannot find method"); >> line 1530: return NULL; // robustness >> Normally "robustness" comments flag logic after an assert() >> to indicate what we do in product mode to deal with the "bad" >> situation without crashing. In this case, we don't try to use >> 'm' after discovering that it is NULL so this could be simpler: >> >> Method* m = InstanceKlass::cast(k)->method_with_idnum(slot); >> assert(m != NULL, "cannot find method"); >> return m; // caller has to deal with NULL in product mode >> >> Yes, I realize you only touched the comment here, but it >> served to point out the messiness of the existing code. >> > > Good suggestion. Fixed. > >> line 1551: return NULL; >> line 1565: return NULL; >> line 1579: return NULL; >> nit: indent should be two spaces >> >> line 1632: return NULL; >> line 1613: return NULL; >> nit: indent should be two spaces > > Fixed all indent nits. > >>> >>> A prototype of the jdk changes can be found here: >>> http://cr.openjdk.java.net/~jfranck/8009382/jdk/webrev.00/ >> >> You should use a separate bug ID for the jdk repo changes. This will >> prevent confusion between when HSX with these changes is integrated >> into JDK8 and when the jdk repo changes are integrated into JDK8. >> > > The JDK changes have a separate bug, sorry if this was unclear. The jdk > changes shown are just a proof of concept, intended to highlight that my > HotSpot changes have actually been run. I will send out a separate review > request in the future for the JDK changes. > > cheers > /Joel
