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
