On Fri, 29 Jul 2022 18:02:46 GMT, Harold Seigel <hsei...@openjdk.org> wrote:
> Please review this change to fix JDK-8291360. This fix adds entry points > getClassFileVersion() and getClassAccessFlagsRaw() to class java.lang.Class. > The new entry points return the current class's class file version and its > raw access flags. > > The fix was tested by running Mach5 tiers 1-2 on Linux, Mac OS, and Windows, > and Mach5 tiers 1-3 on Linux x64. Additionally, the JCK lang, vm, and api > tests and new regression tests were run locally on Linux x64. > > Thanks, Harold Hi Harold, Generally seems fine. A few nits and comments. Thanks. src/hotspot/share/include/jvm.h line 1163: > 1161: > 1162: /* > 1163: * Value types support. Value types? This is supporting the core reflection work isn't it? src/hotspot/share/prims/jvm.cpp line 4050: > 4048: /* > 4049: * Return the current class's class file version. The low order 16 > bits of the > 4050: * the returned jint contains the class's major version. The high > order 16 bits typo "the the" across the lines typo s/contains/contain/ src/hotspot/share/prims/jvm.cpp line 4051: > 4049: * Return the current class's class file version. The low order 16 > bits of the > 4050: * the returned jint contains the class's major version. The high > order 16 bits > 4051: * contains the class's minor version. typo s/contains/contain/ src/hotspot/share/prims/jvm.cpp line 4064: > 4062: assert(c->is_instance_klass(), "must be"); > 4063: InstanceKlass* ik = InstanceKlass::cast(c); > 4064: return (ik->minor_version() << 16) | ik->major_version(); I'm curious why the format is minor:major rather than major:minor ? src/java.base/share/classes/java/lang/Class.java line 4698: > 4696: * > 4697: * If the class is an array type then the access flags of the > component type is > 4698: * returned. If the class is a primitive then ACC_ABSTRACT | > ACC_FINAL | ACC_PUBLIC. The `ACC_ABSTRACT` seems odd - is that way of indicating this "class" can't be instantiated? Is there some spec document that explains this choice? test/hotspot/jtreg/runtime/ClassFile/ClassAccessFlagsRawTest.java line 60: > 58: int flags = (int)m.invoke((new int[3]).getClass()); > 59: if (flags != 1041) { > 60: throw new RuntimeException("expected 1041, got " + flags + " > for primitive array"); Hex output would be clearer here too. test/hotspot/jtreg/runtime/ClassFile/ClassAccessFlagsRawTest.java line 66: > 64: flags = (int)m.invoke((new SUPERnotset[2]).getClass()); > 65: if (flags != 1) { > 66: throw new RuntimeException("expected 1, got " + flags + " for > object array"); Again hex output would be clearer test/hotspot/jtreg/runtime/ClassFile/ClassFileVersionTest.java line 31: > 29: * @modules java.base/java.lang:open > 30: * @compile classFileVersions.jcod > 31: * @run main/othervm --enable-preview ClassFileVersionTest What preview feature is being used here? test/hotspot/jtreg/runtime/ClassFile/ClassFileVersionTest.java line 45: > 43: if (ver != expectedResult) { > 44: throw new RuntimeException( > 45: "expected " + expectedResult + ", got " + ver + " for > class " + className); It would be clearer to show the expected and actual in minor:major format. That way if the test fails we can easily see which bit is wrong. test/hotspot/jtreg/runtime/ClassFile/ClassFileVersionTest.java line 55: > 53: > 54: testIt("Version64", 64); > 55: testIt("Version59", 59); Any particular reason to choose 59? Shouldn't there also be tests for non-zero minor versions? test/hotspot/jtreg/runtime/ClassFile/ClassFileVersionTest.java line 62: > 60: int ver = (int)m.invoke((new int[3]).getClass()); > 61: if (ver != 64) { > 62: throw new RuntimeException("expected 64, got " + ver + " for > primitive array"); Again minor:major format. ------------- Changes requested by dholmes (Reviewer). PR: https://git.openjdk.org/jdk/pull/9688