Re: RFR: JDK-8225056 VM support for sealed classes
Hi David,
Thanks for reviewing the latest changes.
I'll create the follow on RFE's once the sealed classes code is in mainline.
Harold
On 5/31/2020 9:34 PM, David Holmes wrote:
Hi Harold,
On 1/06/2020 8:57 am, Harold Seigel wrote:
Thanks for the comments.
Here's version 3 of the JDK and VM changes for sealed classes.
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.3/webrev/
The new webrev contains just the following three changes:
1. The sealed classes API's in Class.java (permittedSubclasses() and
isSealed()) were revised and, in particular, API
permittedSubclasses() no longer uses reflection.
For those following along we have presently abandoned the attempt to
cache the array in ReflectionData.
Current changes look okay. But I note from the CSR there appears to be
a further minor update to the javadoc coming.
2. An unneeded 'if' statement was removed from
JVM_GetPermittedSubclasses() (pointed out by David.)
Looks good.
3. VM runtime test files SealedUnnamedModuleIntfTest.java and
Permitted.java were changed to add a test case for a non-public
permitted subclass and its sealed superclass being in the same
module and package.
Looks good.
Additionally, two follow on RFE's will be filed. One to add
additional VM sealed classes tests
Thanks. I think there is a more mechanical approach to testing here
that will allow the complete matrix to be easily covered with minimal
duplication between testing for named and unnamed modules.
and one to improve the implementations of the sealed classes API's in
Class.java.
Thanks.
David
-
Thanks, Harold
On 5/28/2020 8:30 PM, David Holmes wrote:
Hi Harold,
Sorry Mandy's comment raised a couple of issues ...
On 29/05/2020 7:12 am, Mandy Chung wrote:
Hi Harold,
On 5/27/20 1:35 PM, Harold Seigel wrote:
Incremental webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/
Class.java
4406 ReflectionData rd = reflectionData();
4407 ClassDesc[] tmp = rd.permittedSubclasses;
4408 if (tmp != null) {
4409 return tmp;
4410 }
4411
4412 if (isArray() || isPrimitive()) {
4413 rd.permittedSubclasses = new ClassDesc[0];
4414 return rd.permittedSubclasses;
4415 }
This causes an array class or primitive type to create a
ReflectionData. It should first check if this is non-sealed class
and returns a constant empty array.
It can't check if this is a non-sealed class as the isSealed() check
calls the above code! But for arrays and primitives which can't be
sealed we should just do:
4412 if (isArray() || isPrimitive()) {
4413 return new ClassDesc[0];
4414 }
But this then made me realize that we need to be creating defensive
copies of the returned arrays, as happens with other APIs that use
ReflectionData.
Backing up a bit I complained that:
public boolean isSealed() {
return permittedSubclasses().length != 0;
}
is a very inefficient way to answer the question as to whether a
class is sealed, so I suggested that the result of
permittedSubclasses() be cached. Caching is not without its own
issues as we are discovering, and when you add in defensive copies
this seems to be trading one inefficiency for another. For nestmates
we don't cache getNestMembers() because we don;t think it will be
called often - it is there to complete the introspection API of
Class rather than being anticipated as used in a regular
programmatic sense. I expect the same is true for
permittedSubclasses(). Do we expect isSealed() to be used often or
is it too just there for completeness? If just for completeness then
perhaps a VM query would be a better compromise on the efficiency
front? Otherwise I can accept the current implementation of
isSealed(), and a non-caching permittedClasses() for this initial
implementation of sealed classes. If efficiency turns out to be a
problem for isSealed() then we can revisit it then.
Thanks,
David
In fact, ReflectionData caches the derived names and reflected
members for performance and also they may be invalidated when the
class is redefined. It might be okay to add
ReflectionData::permittedSubclasses while `PermittedSubclasses`
attribute can't be redefined and getting this attribute is not
performance sensitive. For example, the result of
`getNestMembers` is not cached in ReflectionData. It may be better
not to add it in ReflectionData for modifiable and
performance-sensitive data.
4421 tmp = new ClassDesc[subclassNames.length];
4422 int i = 0;
4423 for (String subclassName : subclassNames) {
4424 try {
4425 tmp[i++] = ClassDesc.of(subclassName.replace('/', '.'));
4426 } catch (IllegalArgumentException iae) {
4427 throw new InternalError("Invalid type in permitted subclasses
information: " + subclassName, iae);
4428 }
4429 }
Nit: rename tmp to some other name e.g. descs
I read the JVMS but it isn't clear to me that the VM
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Harold,
On 1/06/2020 8:57 am, Harold Seigel wrote:
Thanks for the comments.
Here's version 3 of the JDK and VM changes for sealed classes.
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.3/webrev/
The new webrev contains just the following three changes:
1. The sealed classes API's in Class.java (permittedSubclasses() and
isSealed()) were revised and, in particular, API
permittedSubclasses() no longer uses reflection.
For those following along we have presently abandoned the attempt to
cache the array in ReflectionData.
Current changes look okay. But I note from the CSR there appears to be a
further minor update to the javadoc coming.
2. An unneeded 'if' statement was removed from
JVM_GetPermittedSubclasses() (pointed out by David.)
Looks good.
3. VM runtime test files SealedUnnamedModuleIntfTest.java and
Permitted.java were changed to add a test case for a non-public
permitted subclass and its sealed superclass being in the same
module and package.
Looks good.
Additionally, two follow on RFE's will be filed. One to add additional
VM sealed classes tests
Thanks. I think there is a more mechanical approach to testing here that
will allow the complete matrix to be easily covered with minimal
duplication between testing for named and unnamed modules.
and one to improve the implementations of the
sealed classes API's in Class.java.
Thanks.
David
-
Thanks, Harold
On 5/28/2020 8:30 PM, David Holmes wrote:
Hi Harold,
Sorry Mandy's comment raised a couple of issues ...
On 29/05/2020 7:12 am, Mandy Chung wrote:
Hi Harold,
On 5/27/20 1:35 PM, Harold Seigel wrote:
Incremental webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/
Class.java
4406 ReflectionData rd = reflectionData();
4407 ClassDesc[] tmp = rd.permittedSubclasses;
4408 if (tmp != null) {
4409 return tmp;
4410 }
4411
4412 if (isArray() || isPrimitive()) {
4413 rd.permittedSubclasses = new ClassDesc[0];
4414 return rd.permittedSubclasses;
4415 }
This causes an array class or primitive type to create a
ReflectionData. It should first check if this is non-sealed class
and returns a constant empty array.
It can't check if this is a non-sealed class as the isSealed() check
calls the above code! But for arrays and primitives which can't be
sealed we should just do:
4412 if (isArray() || isPrimitive()) {
4413 return new ClassDesc[0];
4414 }
But this then made me realize that we need to be creating defensive
copies of the returned arrays, as happens with other APIs that use
ReflectionData.
Backing up a bit I complained that:
public boolean isSealed() {
return permittedSubclasses().length != 0;
}
is a very inefficient way to answer the question as to whether a class
is sealed, so I suggested that the result of permittedSubclasses() be
cached. Caching is not without its own issues as we are discovering,
and when you add in defensive copies this seems to be trading one
inefficiency for another. For nestmates we don't cache
getNestMembers() because we don;t think it will be called often - it
is there to complete the introspection API of Class rather than being
anticipated as used in a regular programmatic sense. I expect the same
is true for permittedSubclasses(). Do we expect isSealed() to be used
often or is it too just there for completeness? If just for
completeness then perhaps a VM query would be a better compromise on
the efficiency front? Otherwise I can accept the current
implementation of isSealed(), and a non-caching permittedClasses() for
this initial implementation of sealed classes. If efficiency turns out
to be a problem for isSealed() then we can revisit it then.
Thanks,
David
In fact, ReflectionData caches the derived names and reflected
members for performance and also they may be invalidated when the
class is redefined. It might be okay to add
ReflectionData::permittedSubclasses while `PermittedSubclasses`
attribute can't be redefined and getting this attribute is not
performance sensitive. For example, the result of `getNestMembers`
is not cached in ReflectionData. It may be better not to add it in
ReflectionData for modifiable and performance-sensitive data.
4421 tmp = new ClassDesc[subclassNames.length];
4422 int i = 0;
4423 for (String subclassName : subclassNames) {
4424 try {
4425 tmp[i++] = ClassDesc.of(subclassName.replace('/', '.'));
4426 } catch (IllegalArgumentException iae) {
4427 throw new InternalError("Invalid type in permitted subclasses
information: " + subclassName, iae);
4428 }
4429 }
Nit: rename tmp to some other name e.g. descs
I read the JVMS but it isn't clear to me that the VM will validate
the names in `PermittedSubclasses`attribute are valid class
descriptors. I see ConstantPool::is_klass_or_reference check but
can't find where it validates the name is
Re: RFR: JDK-8225056 VM support for sealed classes
Thanks for the comments.
Here's version 3 of the JDK and VM changes for sealed classes.
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.3/webrev/
The new webrev contains just the following three changes:
1. The sealed classes API's in Class.java (permittedSubclasses() and
isSealed()) were revised and, in particular, API
permittedSubclasses() no longer uses reflection.
2. An unneeded 'if' statement was removed from
JVM_GetPermittedSubclasses() (pointed out by David.)
3. VM runtime test files SealedUnnamedModuleIntfTest.java and
Permitted.java were changed to add a test case for a non-public
permitted subclass and its sealed superclass being in the same
module and package.
Additionally, two follow on RFE's will be filed. One to add additional
VM sealed classes tests and one to improve the implementations of the
sealed classes API's in Class.java.
Thanks, Harold
On 5/28/2020 8:30 PM, David Holmes wrote:
Hi Harold,
Sorry Mandy's comment raised a couple of issues ...
On 29/05/2020 7:12 am, Mandy Chung wrote:
Hi Harold,
On 5/27/20 1:35 PM, Harold Seigel wrote:
Incremental webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/
Class.java
4406 ReflectionData rd = reflectionData();
4407 ClassDesc[] tmp = rd.permittedSubclasses;
4408 if (tmp != null) {
4409 return tmp;
4410 }
4411
4412 if (isArray() || isPrimitive()) {
4413 rd.permittedSubclasses = new ClassDesc[0];
4414 return rd.permittedSubclasses;
4415 }
This causes an array class or primitive type to create a
ReflectionData. It should first check if this is non-sealed class
and returns a constant empty array.
It can't check if this is a non-sealed class as the isSealed() check
calls the above code! But for arrays and primitives which can't be
sealed we should just do:
4412 if (isArray() || isPrimitive()) {
4413 return new ClassDesc[0];
4414 }
But this then made me realize that we need to be creating defensive
copies of the returned arrays, as happens with other APIs that use
ReflectionData.
Backing up a bit I complained that:
public boolean isSealed() {
return permittedSubclasses().length != 0;
}
is a very inefficient way to answer the question as to whether a class
is sealed, so I suggested that the result of permittedSubclasses() be
cached. Caching is not without its own issues as we are discovering,
and when you add in defensive copies this seems to be trading one
inefficiency for another. For nestmates we don't cache
getNestMembers() because we don;t think it will be called often - it
is there to complete the introspection API of Class rather than being
anticipated as used in a regular programmatic sense. I expect the same
is true for permittedSubclasses(). Do we expect isSealed() to be used
often or is it too just there for completeness? If just for
completeness then perhaps a VM query would be a better compromise on
the efficiency front? Otherwise I can accept the current
implementation of isSealed(), and a non-caching permittedClasses() for
this initial implementation of sealed classes. If efficiency turns out
to be a problem for isSealed() then we can revisit it then.
Thanks,
David
In fact, ReflectionData caches the derived names and reflected
members for performance and also they may be invalidated when the
class is redefined. It might be okay to add
ReflectionData::permittedSubclasses while `PermittedSubclasses`
attribute can't be redefined and getting this attribute is not
performance sensitive. For example, the result of `getNestMembers`
is not cached in ReflectionData. It may be better not to add it in
ReflectionData for modifiable and performance-sensitive data.
4421 tmp = new ClassDesc[subclassNames.length];
4422 int i = 0;
4423 for (String subclassName : subclassNames) {
4424 try {
4425 tmp[i++] = ClassDesc.of(subclassName.replace('/', '.'));
4426 } catch (IllegalArgumentException iae) {
4427 throw new InternalError("Invalid type in permitted subclasses
information: " + subclassName, iae);
4428 }
4429 }
Nit: rename tmp to some other name e.g. descs
I read the JVMS but it isn't clear to me that the VM will validate
the names in `PermittedSubclasses`attribute are valid class
descriptors. I see ConstantPool::is_klass_or_reference check but
can't find where it validates the name is a valid class descriptor -
can you point me there? (otherwise, maybe define it to be unspecified?)
W.r.t. the APIs. I don't want to delay this review. I see that you
renamed the method to new API style as I brought up. OTOH, I expect
more discussion is needed. Below is a recent comment from John on
this topic [1]
One comment, really for the future, regarding the shape of the Java
API here: It uses Optional and omits the "get" prefix on accessors.
This is the New Style, as opposed to the Classic Style using null
(for "empty"
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Harold,
Sorry Mandy's comment raised a couple of issues ...
On 29/05/2020 7:12 am, Mandy Chung wrote:
Hi Harold,
On 5/27/20 1:35 PM, Harold Seigel wrote:
Incremental webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/
Class.java
4406 ReflectionData rd = reflectionData();
4407 ClassDesc[] tmp = rd.permittedSubclasses;
4408 if (tmp != null) {
4409 return tmp;
4410 }
4411
4412 if (isArray() || isPrimitive()) {
4413 rd.permittedSubclasses = new ClassDesc[0];
4414 return rd.permittedSubclasses;
4415 }
This causes an array class or primitive type to create a
ReflectionData. It should first check if this is non-sealed class
and returns a constant empty array.
It can't check if this is a non-sealed class as the isSealed() check
calls the above code! But for arrays and primitives which can't be
sealed we should just do:
4412 if (isArray() || isPrimitive()) {
4413 return new ClassDesc[0];
4414 }
But this then made me realize that we need to be creating defensive
copies of the returned arrays, as happens with other APIs that use
ReflectionData.
Backing up a bit I complained that:
public boolean isSealed() {
return permittedSubclasses().length != 0;
}
is a very inefficient way to answer the question as to whether a class
is sealed, so I suggested that the result of permittedSubclasses() be
cached. Caching is not without its own issues as we are discovering, and
when you add in defensive copies this seems to be trading one
inefficiency for another. For nestmates we don't cache getNestMembers()
because we don;t think it will be called often - it is there to complete
the introspection API of Class rather than being anticipated as used in
a regular programmatic sense. I expect the same is true for
permittedSubclasses(). Do we expect isSealed() to be used often or is it
too just there for completeness? If just for completeness then perhaps a
VM query would be a better compromise on the efficiency front? Otherwise
I can accept the current implementation of isSealed(), and a non-caching
permittedClasses() for this initial implementation of sealed classes. If
efficiency turns out to be a problem for isSealed() then we can revisit
it then.
Thanks,
David
In fact, ReflectionData caches the derived names and reflected members
for performance and also they may be invalidated when the class is
redefined. It might be okay to add
ReflectionData::permittedSubclasses while `PermittedSubclasses`
attribute can't be redefined and getting this attribute is not
performance sensitive. For example, the result of `getNestMembers`
is not cached in ReflectionData. It may be better not to add it in
ReflectionData for modifiable and performance-sensitive data.
4421 tmp = new ClassDesc[subclassNames.length];
4422 int i = 0;
4423 for (String subclassName : subclassNames) {
4424 try {
4425 tmp[i++] = ClassDesc.of(subclassName.replace('/', '.'));
4426 } catch (IllegalArgumentException iae) {
4427 throw new InternalError("Invalid type in permitted subclasses
information: " + subclassName, iae);
4428 }
4429 }
Nit: rename tmp to some other name e.g. descs
I read the JVMS but it isn't clear to me that the VM will validate the
names in `PermittedSubclasses`attribute are valid class descriptors.
I see ConstantPool::is_klass_or_reference check but can't find where
it validates the name is a valid class descriptor - can you point me
there? (otherwise, maybe define it to be unspecified?)
W.r.t. the APIs. I don't want to delay this review. I see that you
renamed the method to new API style as I brought up. OTOH, I expect
more discussion is needed. Below is a recent comment from John on this
topic [1]
One comment, really for the future, regarding the shape of the Java
API here: It uses Optional and omits the "get" prefix on accessors.
This is the New Style, as opposed to the Classic Style using null
(for "empty" results) and a "get" prefix ("getComponentType") to get
a related type. We may choose to to use the New Style for new
reflection API points, and if so let's not choose N different New
Styles, but one New Style. Personally I like removing "get"; I accept
Optional instead of null; and I also suggest that arrays (if any) be
replaced by (immutable) Lists in the New Style
There are a few existing Class APIs that use the new API style:
Class arrayClass();
Optional describeConstable();
String descriptorString();
This will set up a precedence of the new API style in this class.
Should this new permittedSubclasses method return a List instead of an
array? It's okay with me if you prefer to revert back to the old API
style and follow this up after integration.
4442 * Returns true if this {@linkplain Class} is sealed.
4443 *
* @return returns true if this class is sealed
NIt: {@code true} instead of true - consistent with the style this
class uses
Re: RFR: JDK-8225056 VM support for sealed classes
Thanks for confirming it.
Mandy
On 5/28/20 3:31 PM, Harold Seigel wrote:
Hi Mandy,
The entries in the PermittedSubclasses attribute are constant pool
ConstantClass_info entries. These names get validated by the VM in
this code in ClassFileParser::parse_constant_pool():
for (index = 1; index < length; index++) {
const jbyte tag = cp->tag_at(index).value();
switch (tag) {
case JVM_CONSTANT_UnresolvedClass: {
const Symbol* const class_name = cp->klass_name_at(index);
// check the name, even if _cp_patches will overwrite it
*verify_legal_class_name(class_name, CHECK);*
break;
}
Thanks, Harold
On 5/28/2020 5:12 PM, Mandy Chung wrote:
I read the JVMS but it isn't clear to me that the VM will validate
the names in `PermittedSubclasses`attribute are valid class
descriptors. I see ConstantPool::is_klass_or_reference check but
can't find where it validates the name is a valid class descriptor -
can you point me there? (otherwise, maybe define it to be unspecified?)
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Mandy,
The entries in the PermittedSubclasses attribute are constant pool
ConstantClass_info entries. These names get validated by the VM in this
code in ClassFileParser::parse_constant_pool():
for (index = 1; index < length; index++) {
const jbyte tag = cp->tag_at(index).value();
switch (tag) {
case JVM_CONSTANT_UnresolvedClass: {
const Symbol* const class_name = cp->klass_name_at(index);
// check the name, even if _cp_patches will overwrite it
*verify_legal_class_name(class_name, CHECK);*
break;
}
Thanks, Harold
On 5/28/2020 5:12 PM, Mandy Chung wrote:
I read the JVMS but it isn't clear to me that the VM will validate the
names in `PermittedSubclasses`attribute are valid class descriptors.
I see ConstantPool::is_klass_or_reference check but can't find where
it validates the name is a valid class descriptor - can you point me
there? (otherwise, maybe define it to be unspecified?)
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Harold,
On 5/27/20 1:35 PM, Harold Seigel wrote:
Incremental webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/
Class.java
4406 ReflectionData rd = reflectionData();
4407 ClassDesc[] tmp = rd.permittedSubclasses;
4408 if (tmp != null) {
4409 return tmp;
4410 }
4411
4412 if (isArray() || isPrimitive()) {
4413 rd.permittedSubclasses = new ClassDesc[0];
4414 return rd.permittedSubclasses;
4415 }
This causes an array class or primitive type to create a
ReflectionData. It should first check if this is non-sealed class and
returns a constant empty array.
In fact, ReflectionData caches the derived names and reflected members
for performance and also they may be invalidated when the class is
redefined. It might be okay to add ReflectionData::permittedSubclasses
while `PermittedSubclasses` attribute can't be redefined and getting
this attribute is not performance sensitive. For example, the result
of `getNestMembers` is not cached in ReflectionData. It may be better
not to add it in ReflectionData for modifiable and performance-sensitive
data.
4421 tmp = new ClassDesc[subclassNames.length];
4422 int i = 0;
4423 for (String subclassName : subclassNames) {
4424 try {
4425 tmp[i++] = ClassDesc.of(subclassName.replace('/', '.'));
4426 } catch (IllegalArgumentException iae) {
4427 throw new InternalError("Invalid type in permitted subclasses
information: " + subclassName, iae);
4428 }
4429 }
Nit: rename tmp to some other name e.g. descs
I read the JVMS but it isn't clear to me that the VM will validate the
names in `PermittedSubclasses`attribute are valid class descriptors. I
see ConstantPool::is_klass_or_reference check but can't find where it
validates the name is a valid class descriptor - can you point me
there? (otherwise, maybe define it to be unspecified?)
W.r.t. the APIs. I don't want to delay this review. I see that you
renamed the method to new API style as I brought up. OTOH, I expect
more discussion is needed. Below is a recent comment from John on this
topic [1]
One comment, really for the future, regarding the shape of the Java
API here: It uses Optional and omits the "get" prefix on accessors.
This is the New Style, as opposed to the Classic Style using null (for
"empty" results) and a "get" prefix ("getComponentType") to get a
related type. We may choose to to use the New Style for new reflection
API points, and if so let's not choose N different New Styles, but one
New Style. Personally I like removing "get"; I accept Optional instead
of null; and I also suggest that arrays (if any) be replaced by
(immutable) Lists in the New Style
There are a few existing Class APIs that use the new API style:
Class arrayClass();
Optional describeConstable();
String descriptorString();
This will set up a precedence of the new API style in this class. Should
this new permittedSubclasses method return a List instead of an array?
It's okay with me if you prefer to revert back to the old API style and
follow this up after integration.
4442 * Returns true if this {@linkplain Class} is sealed.
4443 *
* @return returns true if this class is sealed
NIt: {@code true} instead of true - consistent with the style this
class uses (in most methods)
test/jdk/java/lang/reflect/sealed_classes/SealedClassesReflectionTest.java
Nit: s/sealed_classes/sealedClasses/
- the test directory/file naming convention use camel case or java
variable name convention.
Thanks
[1] https://github.com/openjdk/valhalla/pull/53#issuecomment-633116043
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Harold,
On 28/05/2020 6:35 am, Harold Seigel wrote:
Hi David,
Please review this updated webrev:
Incremental webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/
Changes look good - thanks!
One minor comment:
src/hotspot/share/prims/jvm.cpp
2159 if (length != 0) {
2160 for (int i = 0; i < length; i++) {
The if statement is not needed as the loop will be skipped if length is 0.
On testing:
test/hotspot/jtreg/runtime/modules/SealedModuleTest.java
test/hotspot/jtreg/runtime/sealedClasses/SealedUnnamedModuleTest.java
test/hotspot/jtreg/runtime/sealedClasses/SealedUnnamedModuleIntfTest.java
You don't seem to have coverage of the full test matrix. For the
combination of "same module or not" x "same package or not" x "public or
not", there should be 8 test cases: 3 failures and 5 successes. Then you
also have "listed in permits clause" versus "not listed in permits clause".
Then you have all that for classes and interfaces.
---
On the question of whether to use ReflectionData or not that is really
question for the core-libs folk to decide.
Thanks,
David
-
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/
It includes the following changes:
* Indentation and simplification changes as suggested below
* If a class is not a valid permitted subclass of its sealed super
then an IncompatibleClassChangeError exception is thrown (as
specified in the JVM Spec) instead of VerifyError.
* Added a check that a non-public subclass must be in the same package
as its sealed super. And added appropriate testing.
* Method Class.permittedSubtypes() was changed.
See also inline comments.
On 5/24/2020 10:28 PM, David Holmes wrote:
Hi Harold,
On 22/05/2020 4:33 am, Harold Seigel wrote:
Hi David,
Thanks for looking at this! Please review this new webrev:
http://cr.openjdk.java.net/~hseigel/webrev.01/webrev/
I'll list all relevant commens here rather than interspersing below so
that it is easier to track. Mostly nits below, other than the
is_permitted_subclass check in the VM, and the use of ReflectionData
in java.lang.Class.
--
src/hotspot/share/classfile/classFileParser.cpp
+ bool ClassFileParser::supports_sealed_types() {
+ return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
+ _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
+ Arguments::enable_preview();
+ }
Nowe there is too little indentation - the subclauses of the
conjunction expression should align[1]
+ bool ClassFileParser::supports_sealed_types() {
+ return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
+ _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
+ Arguments::enable_preview();
+ }
Fixed. Along with indentation of supports_records().
3791 if (parsed_permitted_subclasses_attribute) {
3792 classfile_parse_error("Multiple
PermittedSubclasses attributes in class file %s", CHECK);
3793 // Classes marked ACC_FINAL cannot have a
PermittedSubclasses attribute.
3794 } else if (_access_flags.is_final()) {
3795 classfile_parse_error("PermittedSubclasses
attribute in final class file %s", CHECK);
3796 } else {
3797 parsed_permitted_subclasses_attribute = true;
3798 }
The indent of the comment at L3793 is wrong, and its placement is
awkward because it relates to the next condition. But we don't have to
use if-else here as any parse error results in immediate return due to
the CHECK macro. So the above can be reformatted as:
3791 if (parsed_permitted_subclasses_attribute) {
3792 classfile_parse_error("Multiple
PermittedSubclasses attributes in class file %s", CHECK);
3793 }
3794 // Classes marked ACC_FINAL cannot have a
PermittedSubclasses attribute.
3795 if (_access_flags.is_final()) {
3796 classfile_parse_error("PermittedSubclasses
attribute in final class file %s", CHECK);
3797 }
3798 parsed_permitted_subclasses_attribute = true;
Done.
---
src/hotspot/share/oops/instanceKlass.cpp
The logic in InstanceKlass::has_as_permitted_subclass still does not
implement the rules specified in the JVMS. It only implements a "same
module" check, whereas the JVMS specifies an accessibility requirement
as well.
Done.
730 bool InstanceKlass::is_sealed() const {
731 return _permitted_subclasses != NULL &&
732 _permitted_subclasses !=
Universe::the_empty_short_array() &&
733 _permitted_subclasses->length() > 0;
734 }
Please align subclauses.
Done.
---
src/hotspot/share/prims/jvm.cpp
2159 objArrayHandle result (THREAD, r);
Please remove space after "result".
Done.
As we will always create and return an arry, if you reverse these two
statements:
2156 if (length != 0) {
2157 objArrayOop
Re: RFR: JDK-8225056 VM support for sealed classes
Hi David,
Please review this updated webrev:
Incremental webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/
It includes the following changes:
* Indentation and simplification changes as suggested below
* If a class is not a valid permitted subclass of its sealed super
then an IncompatibleClassChangeError exception is thrown (as
specified in the JVM Spec) instead of VerifyError.
* Added a check that a non-public subclass must be in the same package
as its sealed super. And added appropriate testing.
* Method Class.permittedSubtypes() was changed.
See also inline comments.
On 5/24/2020 10:28 PM, David Holmes wrote:
Hi Harold,
On 22/05/2020 4:33 am, Harold Seigel wrote:
Hi David,
Thanks for looking at this! Please review this new webrev:
http://cr.openjdk.java.net/~hseigel/webrev.01/webrev/
I'll list all relevant commens here rather than interspersing below so
that it is easier to track. Mostly nits below, other than the
is_permitted_subclass check in the VM, and the use of ReflectionData
in java.lang.Class.
--
src/hotspot/share/classfile/classFileParser.cpp
+ bool ClassFileParser::supports_sealed_types() {
+ return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
+ _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
+ Arguments::enable_preview();
+ }
Nowe there is too little indentation - the subclauses of the
conjunction expression should align[1]
+ bool ClassFileParser::supports_sealed_types() {
+ return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
+ _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
+ Arguments::enable_preview();
+ }
Fixed. Along with indentation of supports_records().
3791 if (parsed_permitted_subclasses_attribute) {
3792 classfile_parse_error("Multiple
PermittedSubclasses attributes in class file %s", CHECK);
3793 // Classes marked ACC_FINAL cannot have a
PermittedSubclasses attribute.
3794 } else if (_access_flags.is_final()) {
3795 classfile_parse_error("PermittedSubclasses
attribute in final class file %s", CHECK);
3796 } else {
3797 parsed_permitted_subclasses_attribute = true;
3798 }
The indent of the comment at L3793 is wrong, and its placement is
awkward because it relates to the next condition. But we don't have to
use if-else here as any parse error results in immediate return due to
the CHECK macro. So the above can be reformatted as:
3791 if (parsed_permitted_subclasses_attribute) {
3792 classfile_parse_error("Multiple
PermittedSubclasses attributes in class file %s", CHECK);
3793 }
3794 // Classes marked ACC_FINAL cannot have a
PermittedSubclasses attribute.
3795 if (_access_flags.is_final()) {
3796 classfile_parse_error("PermittedSubclasses
attribute in final class file %s", CHECK);
3797 }
3798 parsed_permitted_subclasses_attribute = true;
Done.
---
src/hotspot/share/oops/instanceKlass.cpp
The logic in InstanceKlass::has_as_permitted_subclass still does not
implement the rules specified in the JVMS. It only implements a "same
module" check, whereas the JVMS specifies an accessibility requirement
as well.
Done.
730 bool InstanceKlass::is_sealed() const {
731 return _permitted_subclasses != NULL &&
732 _permitted_subclasses !=
Universe::the_empty_short_array() &&
733 _permitted_subclasses->length() > 0;
734 }
Please align subclauses.
Done.
---
src/hotspot/share/prims/jvm.cpp
2159 objArrayHandle result (THREAD, r);
Please remove space after "result".
Done.
As we will always create and return an arry, if you reverse these two
statements:
2156 if (length != 0) {
2157 objArrayOop r =
oopFactory::new_objArray(SystemDictionary::String_klass(),
2158 length, CHECK_NULL);
and these two:
2169 return (jobjectArray)JNIHandles::make_local(THREAD, result());
2170 }
then you can delete
2172 // if it gets to here return an empty array, cases will be: the
class is primitive, or an array, or just not sealed
2173 objArrayOop result =
oopFactory::new_objArray(SystemDictionary::String_klass(), 0,
CHECK_NULL);
2174 return (jobjectArray)JNIHandles::make_local(env, result);
The comment there is no longer accurate anyway.
Done.
---
src/hotspot/share/prims/jvmtiRedefineClasses.cpp
857 static jvmtiError
check_permitted_subclasses_attribute(InstanceKlass* the_class,
858 InstanceKlass* scratch_class) {
Please align.
Done.
---
src/hotspot/share/prims/jvmtiRedefineClasses.cpp
2007 if (permitted_subclasses != NULL) {
permitted_subclasses cannot be NULL. I initially thought the bug was
in the
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Harold,
On 22/05/2020 4:33 am, Harold Seigel wrote:
Hi David,
Thanks for looking at this! Please review this new webrev:
http://cr.openjdk.java.net/~hseigel/webrev.01/webrev/
I'll list all relevant commens here rather than interspersing below so
that it is easier to track. Mostly nits below, other than the
is_permitted_subclass check in the VM, and the use of ReflectionData in
java.lang.Class.
--
src/hotspot/share/classfile/classFileParser.cpp
+ bool ClassFileParser::supports_sealed_types() {
+ return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
+ _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
+ Arguments::enable_preview();
+ }
Nowe there is too little indentation - the subclauses of the conjunction
expression should align[1]
+ bool ClassFileParser::supports_sealed_types() {
+ return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
+ _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
+ Arguments::enable_preview();
+ }
3791 if (parsed_permitted_subclasses_attribute) {
3792 classfile_parse_error("Multiple
PermittedSubclasses attributes in class file %s", CHECK);
3793 // Classes marked ACC_FINAL cannot have a
PermittedSubclasses attribute.
3794 } else if (_access_flags.is_final()) {
3795 classfile_parse_error("PermittedSubclasses
attribute in final class file %s", CHECK);
3796 } else {
3797 parsed_permitted_subclasses_attribute = true;
3798 }
The indent of the comment at L3793 is wrong, and its placement is
awkward because it relates to the next condition. But we don't have to
use if-else here as any parse error results in immediate return due to
the CHECK macro. So the above can be reformatted as:
3791 if (parsed_permitted_subclasses_attribute) {
3792 classfile_parse_error("Multiple
PermittedSubclasses attributes in class file %s", CHECK);
3793 }
3794 // Classes marked ACC_FINAL cannot have a
PermittedSubclasses attribute.
3795 if (_access_flags.is_final()) {
3796 classfile_parse_error("PermittedSubclasses
attribute in final class file %s", CHECK);
3797 }
3798 parsed_permitted_subclasses_attribute = true;
---
src/hotspot/share/oops/instanceKlass.cpp
The logic in InstanceKlass::has_as_permitted_subclass still does not
implement the rules specified in the JVMS. It only implements a "same
module" check, whereas the JVMS specifies an accessibility requirement
as well.
730 bool InstanceKlass::is_sealed() const {
731 return _permitted_subclasses != NULL &&
732 _permitted_subclasses != Universe::the_empty_short_array() &&
733 _permitted_subclasses->length() > 0;
734 }
Please align subclauses.
---
src/hotspot/share/prims/jvm.cpp
2159 objArrayHandle result (THREAD, r);
Please remove space after "result".
As we will always create and return an arry, if you reverse these two
statements:
2156 if (length != 0) {
2157 objArrayOop r =
oopFactory::new_objArray(SystemDictionary::String_klass(),
2158length, CHECK_NULL);
and these two:
2169 return (jobjectArray)JNIHandles::make_local(THREAD, result());
2170 }
then you can delete
2172 // if it gets to here return an empty array, cases will be: the
class is primitive, or an array, or just not sealed
2173 objArrayOop result =
oopFactory::new_objArray(SystemDictionary::String_klass(), 0, CHECK_NULL);
2174 return (jobjectArray)JNIHandles::make_local(env, result);
The comment there is no longer accurate anyway.
---
src/hotspot/share/prims/jvmtiRedefineClasses.cpp
857 static jvmtiError
check_permitted_subclasses_attribute(InstanceKlass* the_class,
858 InstanceKlass*
scratch_class) {
Please align.
---
src/hotspot/share/prims/jvmtiRedefineClasses.cpp
2007 if (permitted_subclasses != NULL) {
permitted_subclasses cannot be NULL. I initially thought the bug was in
the nest_members version of this code, but they both have the same
properties: the member is initialized to NULL when the InstanceKlass is
constructed, and set to either the proper array or the empty_array()
when classfile parsing is complete. So redefinition cannot encounter a
NULL value here.
---
src/java.base/share/classes/java/lang/Class.java
The use of ReflectionData is not correctly implemented. The
ReflectionData instance is not constant but can be replaced when class
redefinition operates. So you cannot do this:
if (rd.permittedSubclasses != null) {
return rd.permittedSubclasses;
}
because you may be returning the permittedSubclasses field of a
different Reflectiondata instance. You need to read the field once into
a local and thereafter use it. Similarly with:
Re: RFR: JDK-8225056 VM support for sealed classes
Thanks Lois!
I'll add the two ResourceMarks before the changes get pushed.
Harold
On 5/22/2020 11:07 AM, Lois Foltan wrote:
On 5/21/2020 2:33 PM, Harold Seigel wrote:
Hi David,
Thanks for looking at this! Please review this new webrev:
http://cr.openjdk.java.net/~hseigel/webrev.01/webrev/
Hi Harold,
I think this webrev looks good! A couple of minor comments:
- oops/instanceKlass.cpp:
line #236, do you need a ResourceMark here? I noticed there is one
above at line #229 for the log_trace call that uses external_name().
- prims/jvmtiRedefineClasses.cpp:
line #878, I think you need a ResourceMark for this method as well
if you invoke the external_name for the log_trace calls and for
NEW_RESOURCE_ARRAY_RETURN_NULL()?
Tests look good.
Thanks,
Lois
This webrev contains the following significant changes:
1. The format/indentation issues in classFileParser.cpp were fixed.
2. Unneeded checks in InstanceKlass::has_as_permitted_subclass() were
removed and the TRAPS parameter was removed.
3. The changes to klassVtable.* and method.* were reverted. Those
changes were from when sealed classes were marked as final, and are
no longer valid.
4. Method getPermittedSubclasses() in Class.java was renamed to
permittedSubclasses() and its implementation was changed.
5. Variables and methods for 'asm' were renamed from
'permittedSubtypes' to 'permittedSubclasses'.
6. Classes for sealed classes tests were changed to start with capital
letters.
7. Changes to langtools tests were removed from this webrev. They are
covered by the javac webrev (JDK-8244556).
8. The CSR's for JVMTI, JDWP, and JDI are in progress.
Please also see comments inline.
On 5/19/2020 1:26 AM, David Holmes wrote:
Hi Harold,
Adding serviceability-dev for the serviceability related changes.
Nit: "VM support for sealed classes"
This RFR covers the VM, core-libs, serviceability and even some
langtools tests. AFAICS only the javac changes are not included here
so when and where will they be reviewed and under what bug id?
Ideally there will be a single JBS issue for "Implementation of JEP
360: Sealed types". It's okay to break up the RFRs across different
areas, but it should be done under one bug id.
The javac changes are being reviewed as bug JDK-8227406. We
understand the need to do the reviews under one bug.
Overall this looks good. I've looked at all files and mainly have
some style nits in various places. But there are some more
significant items below.
On 14/05/2020 7:09 am, Harold Seigel wrote:
Hi,
Please review this patch for JVM and Core-libs support for the JEP
360 Sealed Classes preview feature. Code changes include the
following:
* Processing of the new PermittedSubclasses attribute to enforce
that
a class or interface, whose super is a sealed class or interface,
must be listed in the super's PermittedSubclasses attribute.
* Disallow redefinition of a sealed class or interface if its
redefinition would change its PermittedSubclasses attribute.
* Support API's to determine if a class or interface is sealed
and, if
it's sealed, return a list of its permitted subclasses.
* asm support for the PermittedSubclasses attribute
I assume Remi is providing the upstream support in ASM? :) But also
see below ...
Open Webrev:
http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html
make/data/jdwp/jdwp.spec
There needs to be a sub-task and associated CSR request for this
JDWP spec update. I couldn't see this covered anywhere.
---
src/hotspot/share/classfile/classFileParser.cpp
3215 u2
ClassFileParser::parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
3216 const u1* const permitted_subclasses_attribute_start,
3217 TRAPS) {
Indention on L3216/17 needs fixing after the copy'n'edit.
3515 return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
3516 _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
3517 Arguments::enable_preview();
Too much indentation on L3516/17
3790 // Check for PermittedSubclasses tag
That comment (copied from my nestmates code :) is in the wrong
place. It needs to be before
3788 if (tag == vmSymbols::tag_permitted_subclasses()) {
Minor nit: I would suggest checking
parsed_permitted_subclasses_attribute before checking ACC_FINAL.
3876 if (parsed_permitted_subclasses_attribute) {
3877 const u2 num_of_subclasses =
parse_classfile_permitted_subclasses_attribute(
3878 cfs,
3879 permitted_subclasses_attribute_start,
3880 CHECK);
Although it looks odd the preceding, similarly shaped, sections all
indent to the same absolute position. Can you make L3878/78/80 match
please.
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
3884 sizeof(num_of_subclasses) + sizeof(u2) *
num_of_subclasses,
3885
Re: RFR: JDK-8225056 VM support for sealed classes
On 5/21/2020 2:33 PM, Harold Seigel wrote:
Hi David,
Thanks for looking at this! Please review this new webrev:
http://cr.openjdk.java.net/~hseigel/webrev.01/webrev/
Hi Harold,
I think this webrev looks good! A couple of minor comments:
- oops/instanceKlass.cpp:
line #236, do you need a ResourceMark here? I noticed there is one
above at line #229 for the log_trace call that uses external_name().
- prims/jvmtiRedefineClasses.cpp:
line #878, I think you need a ResourceMark for this method as well if
you invoke the external_name for the log_trace calls and for
NEW_RESOURCE_ARRAY_RETURN_NULL()?
Tests look good.
Thanks,
Lois
This webrev contains the following significant changes:
1. The format/indentation issues in classFileParser.cpp were fixed.
2. Unneeded checks in InstanceKlass::has_as_permitted_subclass() were
removed and the TRAPS parameter was removed.
3. The changes to klassVtable.* and method.* were reverted. Those
changes were from when sealed classes were marked as final, and are
no longer valid.
4. Method getPermittedSubclasses() in Class.java was renamed to
permittedSubclasses() and its implementation was changed.
5. Variables and methods for 'asm' were renamed from
'permittedSubtypes' to 'permittedSubclasses'.
6. Classes for sealed classes tests were changed to start with capital
letters.
7. Changes to langtools tests were removed from this webrev. They are
covered by the javac webrev (JDK-8244556).
8. The CSR's for JVMTI, JDWP, and JDI are in progress.
Please also see comments inline.
On 5/19/2020 1:26 AM, David Holmes wrote:
Hi Harold,
Adding serviceability-dev for the serviceability related changes.
Nit: "VM support for sealed classes"
This RFR covers the VM, core-libs, serviceability and even some
langtools tests. AFAICS only the javac changes are not included here
so when and where will they be reviewed and under what bug id?
Ideally there will be a single JBS issue for "Implementation of JEP
360: Sealed types". It's okay to break up the RFRs across different
areas, but it should be done under one bug id.
The javac changes are being reviewed as bug JDK-8227406. We
understand the need to do the reviews under one bug.
Overall this looks good. I've looked at all files and mainly have
some style nits in various places. But there are some more
significant items below.
On 14/05/2020 7:09 am, Harold Seigel wrote:
Hi,
Please review this patch for JVM and Core-libs support for the JEP
360 Sealed Classes preview feature. Code changes include the
following:
* Processing of the new PermittedSubclasses attribute to enforce that
a class or interface, whose super is a sealed class or interface,
must be listed in the super's PermittedSubclasses attribute.
* Disallow redefinition of a sealed class or interface if its
redefinition would change its PermittedSubclasses attribute.
* Support API's to determine if a class or interface is sealed
and, if
it's sealed, return a list of its permitted subclasses.
* asm support for the PermittedSubclasses attribute
I assume Remi is providing the upstream support in ASM? :) But also
see below ...
Open Webrev:
http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html
make/data/jdwp/jdwp.spec
There needs to be a sub-task and associated CSR request for this JDWP
spec update. I couldn't see this covered anywhere.
---
src/hotspot/share/classfile/classFileParser.cpp
3215 u2
ClassFileParser::parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
3216 const u1* const permitted_subclasses_attribute_start,
3217 TRAPS) {
Indention on L3216/17 needs fixing after the copy'n'edit.
3515 return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
3516 _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
3517 Arguments::enable_preview();
Too much indentation on L3516/17
3790 // Check for PermittedSubclasses tag
That comment (copied from my nestmates code :) is in the wrong place.
It needs to be before
3788 if (tag == vmSymbols::tag_permitted_subclasses()) {
Minor nit: I would suggest checking
parsed_permitted_subclasses_attribute before checking ACC_FINAL.
3876 if (parsed_permitted_subclasses_attribute) {
3877 const u2 num_of_subclasses =
parse_classfile_permitted_subclasses_attribute(
3878 cfs,
3879 permitted_subclasses_attribute_start,
3880 CHECK);
Although it looks odd the preceding, similarly shaped, sections all
indent to the same absolute position. Can you make L3878/78/80 match
please.
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
3884 sizeof(num_of_subclasses) + sizeof(u2) *
num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class
file %s", CHECK);
Nits: please reformat as:
3882 guarantee_property(
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Mandy,
Thanks for the suggestions. They have been incorporated in the revised
webrev.
http://cr.openjdk.java.net/~hseigel/webrev.01/webrev/
Harold
On 5/20/2020 1:05 PM, Mandy Chung wrote:
Hi Vicente,
On 5/20/20 8:40 AM, Vicente Romero wrote:
Hi David,
src/java.base/share/classes/java/lang/Class.java
There needs to be a CSR request for these changes.
yes there is one already:
https://bugs.openjdk.java.net/browse/JDK-8244556
Adding to David's comment w.r.t. @throws IAE:
The Class::getXXX APIs returns `Class` or `Class[]` if the result is
about type(s). This new `getPermittedSubclasses` returns
`ClassDesc[]`. I wonder if this should be renamed to
`permittedSubclasses` to follow the convention of the new APIs added
to support descriptors e.g. `describeConstable`
Nit: {@linkplain Class} should be {@code Class}
Mandy
Re: RFR: JDK-8225056 VM support for sealed classes
Hi David,
Thanks for looking at this! Please review this new webrev:
http://cr.openjdk.java.net/~hseigel/webrev.01/webrev/
This webrev contains the following significant changes:
1. The format/indentation issues in classFileParser.cpp were fixed.
2. Unneeded checks in InstanceKlass::has_as_permitted_subclass() were
removed and the TRAPS parameter was removed.
3. The changes to klassVtable.* and method.* were reverted. Those
changes were from when sealed classes were marked as final, and are
no longer valid.
4. Method getPermittedSubclasses() in Class.java was renamed to
permittedSubclasses() and its implementation was changed.
5. Variables and methods for 'asm' were renamed from
'permittedSubtypes' to 'permittedSubclasses'.
6. Classes for sealed classes tests were changed to start with capital
letters.
7. Changes to langtools tests were removed from this webrev. They are
covered by the javac webrev (JDK-8244556).
8. The CSR's for JVMTI, JDWP, and JDI are in progress.
Please also see comments inline.
On 5/19/2020 1:26 AM, David Holmes wrote:
Hi Harold,
Adding serviceability-dev for the serviceability related changes.
Nit: "VM support for sealed classes"
This RFR covers the VM, core-libs, serviceability and even some
langtools tests. AFAICS only the javac changes are not included here
so when and where will they be reviewed and under what bug id? Ideally
there will be a single JBS issue for "Implementation of JEP 360:
Sealed types". It's okay to break up the RFRs across different areas,
but it should be done under one bug id.
The javac changes are being reviewed as bug JDK-8227406. We understand
the need to do the reviews under one bug.
Overall this looks good. I've looked at all files and mainly have some
style nits in various places. But there are some more significant
items below.
On 14/05/2020 7:09 am, Harold Seigel wrote:
Hi,
Please review this patch for JVM and Core-libs support for the JEP
360 Sealed Classes preview feature. Code changes include the following:
* Processing of the new PermittedSubclasses attribute to enforce that
a class or interface, whose super is a sealed class or interface,
must be listed in the super's PermittedSubclasses attribute.
* Disallow redefinition of a sealed class or interface if its
redefinition would change its PermittedSubclasses attribute.
* Support API's to determine if a class or interface is sealed and, if
it's sealed, return a list of its permitted subclasses.
* asm support for the PermittedSubclasses attribute
I assume Remi is providing the upstream support in ASM? :) But also
see below ...
Open Webrev:
http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html
make/data/jdwp/jdwp.spec
There needs to be a sub-task and associated CSR request for this JDWP
spec update. I couldn't see this covered anywhere.
---
src/hotspot/share/classfile/classFileParser.cpp
3215 u2
ClassFileParser::parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
3216 const u1* const permitted_subclasses_attribute_start,
3217 TRAPS) {
Indention on L3216/17 needs fixing after the copy'n'edit.
3515 return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
3516 _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
3517 Arguments::enable_preview();
Too much indentation on L3516/17
3790 // Check for PermittedSubclasses tag
That comment (copied from my nestmates code :) is in the wrong place.
It needs to be before
3788 if (tag == vmSymbols::tag_permitted_subclasses()) {
Minor nit: I would suggest checking
parsed_permitted_subclasses_attribute before checking ACC_FINAL.
3876 if (parsed_permitted_subclasses_attribute) {
3877 const u2 num_of_subclasses =
parse_classfile_permitted_subclasses_attribute(
3878 cfs,
3879 permitted_subclasses_attribute_start,
3880 CHECK);
Although it looks odd the preceding, similarly shaped, sections all
indent to the same absolute position. Can you make L3878/78/80 match
please.
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
3884 sizeof(num_of_subclasses) + sizeof(u2) *
num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class file
%s", CHECK);
Nits: please reformat as:
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
sizeof(num_of_subclasses) + sizeof(u2) * num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class file
%s", CHECK);
It would also look slightly better if you shortened the name of the
num_of_subclasses variable.
All of the above classFileParser.cpp changes were done.
---
src/hotspot/share/classfile/classFileParser.hpp
+ u2 parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
+
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Vicente,
On 21/05/2020 1:40 am, Vicente Romero wrote:
Hi David,
src/java.base/share/classes/java/lang/Class.java
There needs to be a CSR request for these changes.
yes there is one already: https://bugs.openjdk.java.net/browse/JDK-8244556
+ * Returns an array containing {@code ClassDesc} objects
representing all the
+ * permitted subclasses of this {@linkplain Class} if it is
sealed. Returns an empty array if this
+ * {@linkplain Class} is not sealed.
should add "or this class represents an array or primitive type"
(using the standard wording for such cases).
well given that array and primitive classes are not sealed classes I
think we are already covered by the method's spec.
Yes, now I've seen the JLS updates, this is more clear to me.
Thanks,
David
-
+ * @throws IllegalArgumentException if a class descriptor is not
in the correct format
IllegalArgumentException is not an appropriate exception to use as
this method takes no arguments. If the class descriptor is not valid
and it comes from the VM then I think we have a problem with how the
VM validates class descriptors. Any IAE from ClassDesc.of should be
caught and converted to a more suitable exception type - preferably
InternalError if the VM should always return valid strings.
we agree with you here, this will be fixed in the next review iteration.
+ public ClassDesc[] getPermittedSubclasses() {
As mentioned for jvm.cpp this Java code should do the isArray() and
isPrimitive() check before calling the VM.
agreed.
Thanks,
Vicente
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Remi, Thank you for reviewing the ASM changes. Harold On 5/19/2020 4:41 AM, Remi Forax wrote: - Mail original - De: "David Holmes" À: "Harold David Seigel" , "hotspot-runtime-dev" , "amber-dev" , "core-libs-dev" , "serviceability-dev" Envoyé: Mardi 19 Mai 2020 07:26:38 Objet: Re: RFR: JDK-8225056 VM support for sealed classes Hi Harold, Adding serviceability-dev for the serviceability related changes. Nit: "VM support for sealed classes" This RFR covers the VM, core-libs, serviceability and even some langtools tests. AFAICS only the javac changes are not included here so when and where will they be reviewed and under what bug id? Ideally there will be a single JBS issue for "Implementation of JEP 360: Sealed types". It's okay to break up the RFRs across different areas, but it should be done under one bug id. Overall this looks good. I've looked at all files and mainly have some style nits in various places. But there are some more significant items below. On 14/05/2020 7:09 am, Harold Seigel wrote: Hi, Please review this patch for JVM and Core-libs support for the JEP 360 Sealed Classes preview feature. Code changes include the following: * Processing of the new PermittedSubclasses attribute to enforce that a class or interface, whose super is a sealed class or interface, must be listed in the super's PermittedSubclasses attribute. * Disallow redefinition of a sealed class or interface if its redefinition would change its PermittedSubclasses attribute. * Support API's to determine if a class or interface is sealed and, if it's sealed, return a list of its permitted subclasses. * asm support for the PermittedSubclasses attribute I assume Remi is providing the upstream support in ASM? :) But also see below ... Currently the version of ASM used JDK already supports sealed classes but with the attribute named PermittedSubtypes instead of PermittedSubclasses. This patch renames the attribute which is the right thing to do. Then when JDK 15 will be released, we will release ASM 9 with full support of PermittedSubclasses, with the right method names, docs, etc, that will be then integrated in JDK 16. So with my ASM hat, the changes to the 3 ASM classes are good. Rémi
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Vicente,
On 5/20/20 8:40 AM, Vicente Romero wrote:
Hi David,
src/java.base/share/classes/java/lang/Class.java
There needs to be a CSR request for these changes.
yes there is one already:
https://bugs.openjdk.java.net/browse/JDK-8244556
Adding to David's comment w.r.t. @throws IAE:
The Class::getXXX APIs returns `Class` or `Class[]` if the result is
about type(s). This new `getPermittedSubclasses` returns
`ClassDesc[]`. I wonder if this should be renamed to
`permittedSubclasses` to follow the convention of the new APIs added to
support descriptors e.g. `describeConstable`
Nit: {@linkplain Class} should be {@code Class}
Mandy
Re: RFR: JDK-8225056 VM support for sealed classes
Hi David,
src/java.base/share/classes/java/lang/Class.java
There needs to be a CSR request for these changes.
yes there is one already: https://bugs.openjdk.java.net/browse/JDK-8244556
+ * Returns an array containing {@code ClassDesc} objects
representing all the
+ * permitted subclasses of this {@linkplain Class} if it is
sealed. Returns an empty array if this
+ * {@linkplain Class} is not sealed.
should add "or this class represents an array or primitive type"
(using the standard wording for such cases).
well given that array and primitive classes are not sealed classes I
think we are already covered by the method's spec.
+ * @throws IllegalArgumentException if a class descriptor is not
in the correct format
IllegalArgumentException is not an appropriate exception to use as
this method takes no arguments. If the class descriptor is not valid
and it comes from the VM then I think we have a problem with how the
VM validates class descriptors. Any IAE from ClassDesc.of should be
caught and converted to a more suitable exception type - preferably
InternalError if the VM should always return valid strings.
we agree with you here, this will be fixed in the next review iteration.
+ public ClassDesc[] getPermittedSubclasses() {
As mentioned for jvm.cpp this Java code should do the isArray() and
isPrimitive() check before calling the VM.
agreed.
Thanks,
Vicente
Re: RFR: JDK-8225056 VM support for sealed classes
Hi David,
On 5/19/20 19:31, David Holmes wrote:
Hi Serguei,
On 20/05/2020 1:49 am, serguei.spit...@oracle.com wrote:
Hi Harold and David,
Just one comment.
We could save on the CSR's for:
make/data/jdwp/jdwp.spec
|| src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java
||
src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java
Just to be clear, you don't need a CSR request per change but each
change must be covered by some CSR request.
Oh, right.
I was confused by your comments against each change.
As we discussed with David, it could make sense to remove duplication
in the Serviceability class redefinition and retransformation specs.
I'd suggest something like this webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/
If the direction looks okay then I could file RFE+CSR (and post an RFR).
I like the idea of keeping one list referred to by the other specs!
Thanks, I've started this.
Probably, it is better to wait until this fix is pushed.
Thanks,
Serguei
Thanks,
David
-
Thanks,
Serguei
On 5/18/20 22:26, David Holmes wrote:
Hi Harold,
Adding serviceability-dev for the serviceability related changes.
Nit: "VM support for sealed classes"
This RFR covers the VM, core-libs, serviceability and even some
langtools tests. AFAICS only the javac changes are not included here
so when and where will they be reviewed and under what bug id?
Ideally there will be a single JBS issue for "Implementation of JEP
360: Sealed types". It's okay to break up the RFRs across different
areas, but it should be done under one bug id.
Overall this looks good. I've looked at all files and mainly have
some style nits in various places. But there are some more
significant items below.
On 14/05/2020 7:09 am, Harold Seigel wrote:
Hi,
Please review this patch for JVM and Core-libs support for the JEP
360 Sealed Classes preview feature. Code changes include the
following:
* Processing of the new PermittedSubclasses attribute to enforce
that
a class or interface, whose super is a sealed class or interface,
must be listed in the super's PermittedSubclasses attribute.
* Disallow redefinition of a sealed class or interface if its
redefinition would change its PermittedSubclasses attribute.
* Support API's to determine if a class or interface is sealed
and, if
it's sealed, return a list of its permitted subclasses.
* asm support for the PermittedSubclasses attribute
I assume Remi is providing the upstream support in ASM? :) But also
see below ...
Open Webrev:
http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html
make/data/jdwp/jdwp.spec
There needs to be a sub-task and associated CSR request for this
JDWP spec update. I couldn't see this covered anywhere.
---
src/hotspot/share/classfile/classFileParser.cpp
3215 u2
ClassFileParser::parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
3216 const u1* const permitted_subclasses_attribute_start,
3217 TRAPS) {
Indention on L3216/17 needs fixing after the copy'n'edit.
3515 return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
3516 _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
3517 Arguments::enable_preview();
Too much indentation on L3516/17
3790 // Check for PermittedSubclasses tag
That comment (copied from my nestmates code :) is in the wrong
place. It needs to be before
3788 if (tag == vmSymbols::tag_permitted_subclasses()) {
Minor nit: I would suggest checking
parsed_permitted_subclasses_attribute before checking ACC_FINAL.
3876 if (parsed_permitted_subclasses_attribute) {
3877 const u2 num_of_subclasses =
parse_classfile_permitted_subclasses_attribute(
3878 cfs,
3879 permitted_subclasses_attribute_start,
3880 CHECK);
Although it looks odd the preceding, similarly shaped, sections all
indent to the same absolute position. Can you make L3878/78/80 match
please.
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
3884 sizeof(num_of_subclasses) + sizeof(u2) *
num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class
file %s", CHECK);
Nits: please reformat as:
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
sizeof(num_of_subclasses) + sizeof(u2) * num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class
file %s", CHECK);
It would also look slightly better if you shortened the name of the
num_of_subclasses variable.
---
src/hotspot/share/classfile/classFileParser.hpp
+ u2 parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
+ const u1* const
permitted_subclasses_attribute_start,
+ TRAPS);
Please
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Serguei,
On 20/05/2020 1:49 am, serguei.spit...@oracle.com wrote:
Hi Harold and David,
Just one comment.
We could save on the CSR's for:
make/data/jdwp/jdwp.spec
|| src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java
||
src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java
Just to be clear, you don't need a CSR request per change but each
change must be covered by some CSR request.
As we discussed with David, it could make sense to remove duplication in
the Serviceability class redefinition and retransformation specs.
I'd suggest something like this webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/
If the direction looks okay then I could file RFE+CSR (and post an RFR).
I like the idea of keeping one list referred to by the other specs!
Thanks,
David
-
Thanks,
Serguei
On 5/18/20 22:26, David Holmes wrote:
Hi Harold,
Adding serviceability-dev for the serviceability related changes.
Nit: "VM support for sealed classes"
This RFR covers the VM, core-libs, serviceability and even some
langtools tests. AFAICS only the javac changes are not included here
so when and where will they be reviewed and under what bug id? Ideally
there will be a single JBS issue for "Implementation of JEP 360:
Sealed types". It's okay to break up the RFRs across different areas,
but it should be done under one bug id.
Overall this looks good. I've looked at all files and mainly have some
style nits in various places. But there are some more significant
items below.
On 14/05/2020 7:09 am, Harold Seigel wrote:
Hi,
Please review this patch for JVM and Core-libs support for the JEP
360 Sealed Classes preview feature. Code changes include the following:
* Processing of the new PermittedSubclasses attribute to enforce that
a class or interface, whose super is a sealed class or interface,
must be listed in the super's PermittedSubclasses attribute.
* Disallow redefinition of a sealed class or interface if its
redefinition would change its PermittedSubclasses attribute.
* Support API's to determine if a class or interface is sealed and, if
it's sealed, return a list of its permitted subclasses.
* asm support for the PermittedSubclasses attribute
I assume Remi is providing the upstream support in ASM? :) But also
see below ...
Open Webrev:
http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html
make/data/jdwp/jdwp.spec
There needs to be a sub-task and associated CSR request for this JDWP
spec update. I couldn't see this covered anywhere.
---
src/hotspot/share/classfile/classFileParser.cpp
3215 u2
ClassFileParser::parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
3216 const u1* const permitted_subclasses_attribute_start,
3217 TRAPS) {
Indention on L3216/17 needs fixing after the copy'n'edit.
3515 return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
3516 _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
3517 Arguments::enable_preview();
Too much indentation on L3516/17
3790 // Check for PermittedSubclasses tag
That comment (copied from my nestmates code :) is in the wrong place.
It needs to be before
3788 if (tag == vmSymbols::tag_permitted_subclasses()) {
Minor nit: I would suggest checking
parsed_permitted_subclasses_attribute before checking ACC_FINAL.
3876 if (parsed_permitted_subclasses_attribute) {
3877 const u2 num_of_subclasses =
parse_classfile_permitted_subclasses_attribute(
3878 cfs,
3879 permitted_subclasses_attribute_start,
3880 CHECK);
Although it looks odd the preceding, similarly shaped, sections all
indent to the same absolute position. Can you make L3878/78/80 match
please.
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
3884 sizeof(num_of_subclasses) + sizeof(u2) *
num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class file
%s", CHECK);
Nits: please reformat as:
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
sizeof(num_of_subclasses) + sizeof(u2) * num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class file
%s", CHECK);
It would also look slightly better if you shortened the name of the
num_of_subclasses variable.
---
src/hotspot/share/classfile/classFileParser.hpp
+ u2 parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
+ const u1* const
permitted_subclasses_attribute_start,
+ TRAPS);
Please fix indentation after copy'n'edit.
---
src/hotspot/share/oops/instanceKlass.cpp
247 if (classloader1 != classloader2) {
I'm not clear what rule this is verifying. The same module check
follows this one. The rule is
Re: RFR: JDK-8225056 VM support for sealed classes
Filed the enhancement:
https://bugs.openjdk.java.net/browse/JDK-8245392
Remove deduplication in class redefinition and
retransformation specs
Will also create CSR and post RFR soon.
Thanks,
Serguei
On 5/19/20 09:46, Harold Seigel wrote:
I think that's a good idea.
Thanks, Harold
On 5/19/2020 11:49 AM, serguei.spit...@oracle.com wrote:
Hi Harold and David,
Just one comment.
We could save on the CSR's for:
make/data/jdwp/jdwp.spec
src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java
src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java
As we discussed with David, it could make sense to remove
duplication in the Serviceability class redefinition and
retransformation specs.
I'd suggest something like this webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/
If the direction looks okay then I could file RFE+CSR (and
post an RFR).
Thanks,
Serguei
On 5/18/20 22:26, David Holmes wrote:
Hi
Harold,
Adding serviceability-dev for the serviceability related
changes.
Nit: "VM support for sealed classes"
This RFR covers the VM, core-libs, serviceability and even
some langtools tests. AFAICS only the javac changes are not
included here so when and where will they be reviewed and
under what bug id? Ideally there will be a single JBS issue
for "Implementation of JEP 360: Sealed types". It's okay to
break up the RFRs across different areas, but it should be
done under one bug id.
Overall this looks good. I've looked at all files and mainly
have some style nits in various places. But there are some
more significant items below.
On 14/05/2020 7:09 am, Harold Seigel wrote:
Hi,
Please review this patch for JVM and Core-libs support for
the JEP 360 Sealed Classes preview feature. Code changes
include the following:
* Processing of the new PermittedSubclasses attribute to
enforce that
a class or interface, whose super is a sealed class or
interface,
must be listed in the super's PermittedSubclasses
attribute.
* Disallow redefinition of a sealed class or interface if
its
redefinition would change its PermittedSubclasses
attribute.
* Support API's to determine if a class or interface is
sealed and, if
it's sealed, return a list of its permitted subclasses.
* asm support for the PermittedSubclasses attribute
I assume Remi is providing the upstream support in ASM? :) But
also see below ...
Open Webrev: http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html
make/data/jdwp/jdwp.spec
There needs to be a sub-task and associated CSR request for
this JDWP spec update. I couldn't see this covered anywhere.
---
src/hotspot/share/classfile/classFileParser.cpp
3215 u2
ClassFileParser::parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
3216 const
u1* const permitted_subclasses_attribute_start,
3217 TRAPS) {
Indention on L3216/17 needs fixing after the copy'n'edit.
3515 return _major_version == JVM_CLASSFILE_MAJOR_VERSION
&&
3516 _minor_version == JAVA_PREVIEW_MINOR_VERSION
&&
3517 Arguments::enable_preview();
Too much indentation on L3516/17
3790 // Check for PermittedSubclasses tag
That comment (copied from my nestmates code :) is in the wrong
place. It needs to be before
3788 if (tag ==
vmSymbols::tag_permitted_subclasses()) {
Minor nit: I would suggest checking
parsed_permitted_subclasses_attribute before checking
ACC_FINAL.
3876 if (parsed_permitted_subclasses_attribute) {
Re: RFR: JDK-8225056 VM support for sealed classes
On 5/19/20 09:46, Harold Seigel wrote:
That sounds good!
Thanks, Harold
On 5/19/2020 11:53 AM, serguei.spit...@oracle.com wrote:
Hi Harold,
The Serviceability part including the tests looks good to me.
I can file a JVMTI enhancement on the jvmtiRedefineClasses.cpp
refactoring if you are okay with it.
Filed enhancement and assigned to myself:
https://bugs.openjdk.java.net/browse/JDK-8245321
refactor the redefine check that an attribute consisting of a list
of classes has not changed
Thanks,
Serguei
Thanks,
Serguei
On 5/18/20 22:26, David Holmes wrote:
Hi Harold,
Adding serviceability-dev for the serviceability related changes.
Nit: "VM support for sealed classes"
This RFR covers the VM, core-libs, serviceability and even some
langtools tests. AFAICS only the javac changes are not included here
so when and where will they be reviewed and under what bug id?
Ideally there will be a single JBS issue for "Implementation of JEP
360: Sealed types". It's okay to break up the RFRs across different
areas, but it should be done under one bug id.
Overall this looks good. I've looked at all files and mainly have
some style nits in various places. But there are some more
significant items below.
On 14/05/2020 7:09 am, Harold Seigel wrote:
Hi,
Please review this patch for JVM and Core-libs support for the JEP
360 Sealed Classes preview feature. Code changes include the
following:
* Processing of the new PermittedSubclasses attribute to enforce
that
a class or interface, whose super is a sealed class or interface,
must be listed in the super's PermittedSubclasses attribute.
* Disallow redefinition of a sealed class or interface if its
redefinition would change its PermittedSubclasses attribute.
* Support API's to determine if a class or interface is sealed
and, if
it's sealed, return a list of its permitted subclasses.
* asm support for the PermittedSubclasses attribute
I assume Remi is providing the upstream support in ASM? :) But also
see below ...
Open Webrev:
http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html
make/data/jdwp/jdwp.spec
There needs to be a sub-task and associated CSR request for this
JDWP spec update. I couldn't see this covered anywhere.
---
src/hotspot/share/classfile/classFileParser.cpp
3215 u2
ClassFileParser::parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
3216 const u1* const permitted_subclasses_attribute_start,
3217 TRAPS) {
Indention on L3216/17 needs fixing after the copy'n'edit.
3515 return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
3516 _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
3517 Arguments::enable_preview();
Too much indentation on L3516/17
3790 // Check for PermittedSubclasses tag
That comment (copied from my nestmates code :) is in the wrong
place. It needs to be before
3788 if (tag == vmSymbols::tag_permitted_subclasses()) {
Minor nit: I would suggest checking
parsed_permitted_subclasses_attribute before checking ACC_FINAL.
3876 if (parsed_permitted_subclasses_attribute) {
3877 const u2 num_of_subclasses =
parse_classfile_permitted_subclasses_attribute(
3878 cfs,
3879 permitted_subclasses_attribute_start,
3880 CHECK);
Although it looks odd the preceding, similarly shaped, sections all
indent to the same absolute position. Can you make L3878/78/80 match
please.
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
3884 sizeof(num_of_subclasses) + sizeof(u2) *
num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class
file %s", CHECK);
Nits: please reformat as:
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
sizeof(num_of_subclasses) + sizeof(u2) * num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class
file %s", CHECK);
It would also look slightly better if you shortened the name of the
num_of_subclasses variable.
---
src/hotspot/share/classfile/classFileParser.hpp
+ u2 parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
+ const u1* const
permitted_subclasses_attribute_start,
+ TRAPS);
Please fix indentation after copy'n'edit.
---
src/hotspot/share/oops/instanceKlass.cpp
247 if (classloader1 != classloader2) {
I'm not clear what rule this is verifying. The same module check
follows this one. The rule is that the subclass must be accessible
to the superclass implying:
1. same named module (regardless of class access modifiers); or
2. (implicitly in un-named module) same package if subclass not
public; or
3. public subclass
Having the same classloader implies same package, but that alone
doesn't address 2 or 3. So
Re: RFR: JDK-8225056 VM support for sealed classes
That sounds good!
Thanks, Harold
On 5/19/2020 11:53 AM, serguei.spit...@oracle.com wrote:
Hi Harold,
The Serviceability part including the tests looks good to me.
I can file a JVMTI enhancement on the jvmtiRedefineClasses.cpp
refactoring if you are okay with it.
Thanks,
Serguei
On 5/18/20 22:26, David Holmes wrote:
Hi Harold,
Adding serviceability-dev for the serviceability related changes.
Nit: "VM support for sealed classes"
This RFR covers the VM, core-libs, serviceability and even some
langtools tests. AFAICS only the javac changes are not included here
so when and where will they be reviewed and under what bug id?
Ideally there will be a single JBS issue for "Implementation of JEP
360: Sealed types". It's okay to break up the RFRs across different
areas, but it should be done under one bug id.
Overall this looks good. I've looked at all files and mainly have
some style nits in various places. But there are some more
significant items below.
On 14/05/2020 7:09 am, Harold Seigel wrote:
Hi,
Please review this patch for JVM and Core-libs support for the JEP
360 Sealed Classes preview feature. Code changes include the
following:
* Processing of the new PermittedSubclasses attribute to enforce that
a class or interface, whose super is a sealed class or interface,
must be listed in the super's PermittedSubclasses attribute.
* Disallow redefinition of a sealed class or interface if its
redefinition would change its PermittedSubclasses attribute.
* Support API's to determine if a class or interface is sealed
and, if
it's sealed, return a list of its permitted subclasses.
* asm support for the PermittedSubclasses attribute
I assume Remi is providing the upstream support in ASM? :) But also
see below ...
Open Webrev:
http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html
make/data/jdwp/jdwp.spec
There needs to be a sub-task and associated CSR request for this JDWP
spec update. I couldn't see this covered anywhere.
---
src/hotspot/share/classfile/classFileParser.cpp
3215 u2
ClassFileParser::parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
3216 const u1* const permitted_subclasses_attribute_start,
3217 TRAPS) {
Indention on L3216/17 needs fixing after the copy'n'edit.
3515 return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
3516 _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
3517 Arguments::enable_preview();
Too much indentation on L3516/17
3790 // Check for PermittedSubclasses tag
That comment (copied from my nestmates code :) is in the wrong place.
It needs to be before
3788 if (tag == vmSymbols::tag_permitted_subclasses()) {
Minor nit: I would suggest checking
parsed_permitted_subclasses_attribute before checking ACC_FINAL.
3876 if (parsed_permitted_subclasses_attribute) {
3877 const u2 num_of_subclasses =
parse_classfile_permitted_subclasses_attribute(
3878 cfs,
3879 permitted_subclasses_attribute_start,
3880 CHECK);
Although it looks odd the preceding, similarly shaped, sections all
indent to the same absolute position. Can you make L3878/78/80 match
please.
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
3884 sizeof(num_of_subclasses) + sizeof(u2) *
num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class
file %s", CHECK);
Nits: please reformat as:
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
sizeof(num_of_subclasses) + sizeof(u2) * num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class
file %s", CHECK);
It would also look slightly better if you shortened the name of the
num_of_subclasses variable.
---
src/hotspot/share/classfile/classFileParser.hpp
+ u2 parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
+ const u1* const
permitted_subclasses_attribute_start,
+ TRAPS);
Please fix indentation after copy'n'edit.
---
src/hotspot/share/oops/instanceKlass.cpp
247 if (classloader1 != classloader2) {
I'm not clear what rule this is verifying. The same module check
follows this one. The rule is that the subclass must be accessible to
the superclass implying:
1. same named module (regardless of class access modifiers); or
2. (implicitly in un-named module) same package if subclass not
public; or
3. public subclass
Having the same classloader implies same package, but that alone
doesn't address 2 or 3. So this doesn't conform to proposed JVMS rules.
264 if (_constants->tag_at(cp_index).is_klass()) {
265 Klass* k2 = _constants->klass_at(cp_index, CHECK_false);
You've copied this code from the nestmember checks but your changes
don't quite
Re: RFR: JDK-8225056 VM support for sealed classes
I think that's a good idea.
Thanks, Harold
On 5/19/2020 11:49 AM, serguei.spit...@oracle.com wrote:
Hi Harold and David,
Just one comment.
We could save on the CSR's for:
make/data/jdwp/jdwp.spec
|| src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java
||
src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java
As we discussed with David, it could make sense to remove duplication
in the Serviceability class redefinition and retransformation specs.
I'd suggest something like this webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/
If the direction looks okay then I could file RFE+CSR (and post an RFR).
Thanks,
Serguei
On 5/18/20 22:26, David Holmes wrote:
Hi Harold,
Adding serviceability-dev for the serviceability related changes.
Nit: "VM support for sealed classes"
This RFR covers the VM, core-libs, serviceability and even some
langtools tests. AFAICS only the javac changes are not included here
so when and where will they be reviewed and under what bug id?
Ideally there will be a single JBS issue for "Implementation of JEP
360: Sealed types". It's okay to break up the RFRs across different
areas, but it should be done under one bug id.
Overall this looks good. I've looked at all files and mainly have
some style nits in various places. But there are some more
significant items below.
On 14/05/2020 7:09 am, Harold Seigel wrote:
Hi,
Please review this patch for JVM and Core-libs support for the JEP
360 Sealed Classes preview feature. Code changes include the
following:
* Processing of the new PermittedSubclasses attribute to enforce that
a class or interface, whose super is a sealed class or interface,
must be listed in the super's PermittedSubclasses attribute.
* Disallow redefinition of a sealed class or interface if its
redefinition would change its PermittedSubclasses attribute.
* Support API's to determine if a class or interface is sealed
and, if
it's sealed, return a list of its permitted subclasses.
* asm support for the PermittedSubclasses attribute
I assume Remi is providing the upstream support in ASM? :) But also
see below ...
Open Webrev:
http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html
make/data/jdwp/jdwp.spec
There needs to be a sub-task and associated CSR request for this JDWP
spec update. I couldn't see this covered anywhere.
---
src/hotspot/share/classfile/classFileParser.cpp
3215 u2
ClassFileParser::parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
3216 const u1* const permitted_subclasses_attribute_start,
3217 TRAPS) {
Indention on L3216/17 needs fixing after the copy'n'edit.
3515 return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
3516 _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
3517 Arguments::enable_preview();
Too much indentation on L3516/17
3790 // Check for PermittedSubclasses tag
That comment (copied from my nestmates code :) is in the wrong place.
It needs to be before
3788 if (tag == vmSymbols::tag_permitted_subclasses()) {
Minor nit: I would suggest checking
parsed_permitted_subclasses_attribute before checking ACC_FINAL.
3876 if (parsed_permitted_subclasses_attribute) {
3877 const u2 num_of_subclasses =
parse_classfile_permitted_subclasses_attribute(
3878 cfs,
3879 permitted_subclasses_attribute_start,
3880 CHECK);
Although it looks odd the preceding, similarly shaped, sections all
indent to the same absolute position. Can you make L3878/78/80 match
please.
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
3884 sizeof(num_of_subclasses) + sizeof(u2) *
num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class
file %s", CHECK);
Nits: please reformat as:
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
sizeof(num_of_subclasses) + sizeof(u2) * num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class
file %s", CHECK);
It would also look slightly better if you shortened the name of the
num_of_subclasses variable.
---
src/hotspot/share/classfile/classFileParser.hpp
+ u2 parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
+ const u1* const
permitted_subclasses_attribute_start,
+ TRAPS);
Please fix indentation after copy'n'edit.
---
src/hotspot/share/oops/instanceKlass.cpp
247 if (classloader1 != classloader2) {
I'm not clear what rule this is verifying. The same module check
follows this one. The rule is that the subclass must be accessible to
the superclass implying:
1. same named module (regardless of class access modifiers); or
2. (implicitly in un-named module) same
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Harold,
The Serviceability part including the tests looks good to me.
I can file a JVMTI enhancement on the jvmtiRedefineClasses.cpp
refactoring if you are okay with it.
Thanks,
Serguei
On 5/18/20 22:26, David Holmes wrote:
Hi Harold,
Adding serviceability-dev for the serviceability related changes.
Nit: "VM support for sealed classes"
This RFR covers the VM, core-libs, serviceability and even some
langtools tests. AFAICS only the javac changes are not included here
so when and where will they be reviewed and under what bug id? Ideally
there will be a single JBS issue for "Implementation of JEP 360:
Sealed types". It's okay to break up the RFRs across different areas,
but it should be done under one bug id.
Overall this looks good. I've looked at all files and mainly have some
style nits in various places. But there are some more significant
items below.
On 14/05/2020 7:09 am, Harold Seigel wrote:
Hi,
Please review this patch for JVM and Core-libs support for the JEP
360 Sealed Classes preview feature. Code changes include the following:
* Processing of the new PermittedSubclasses attribute to enforce that
a class or interface, whose super is a sealed class or interface,
must be listed in the super's PermittedSubclasses attribute.
* Disallow redefinition of a sealed class or interface if its
redefinition would change its PermittedSubclasses attribute.
* Support API's to determine if a class or interface is sealed and, if
it's sealed, return a list of its permitted subclasses.
* asm support for the PermittedSubclasses attribute
I assume Remi is providing the upstream support in ASM? :) But also
see below ...
Open Webrev:
http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html
make/data/jdwp/jdwp.spec
There needs to be a sub-task and associated CSR request for this JDWP
spec update. I couldn't see this covered anywhere.
---
src/hotspot/share/classfile/classFileParser.cpp
3215 u2
ClassFileParser::parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
3216 const u1* const permitted_subclasses_attribute_start,
3217 TRAPS) {
Indention on L3216/17 needs fixing after the copy'n'edit.
3515 return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
3516 _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
3517 Arguments::enable_preview();
Too much indentation on L3516/17
3790 // Check for PermittedSubclasses tag
That comment (copied from my nestmates code :) is in the wrong place.
It needs to be before
3788 if (tag == vmSymbols::tag_permitted_subclasses()) {
Minor nit: I would suggest checking
parsed_permitted_subclasses_attribute before checking ACC_FINAL.
3876 if (parsed_permitted_subclasses_attribute) {
3877 const u2 num_of_subclasses =
parse_classfile_permitted_subclasses_attribute(
3878 cfs,
3879 permitted_subclasses_attribute_start,
3880 CHECK);
Although it looks odd the preceding, similarly shaped, sections all
indent to the same absolute position. Can you make L3878/78/80 match
please.
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
3884 sizeof(num_of_subclasses) + sizeof(u2) *
num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class file
%s", CHECK);
Nits: please reformat as:
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
sizeof(num_of_subclasses) + sizeof(u2) * num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class file
%s", CHECK);
It would also look slightly better if you shortened the name of the
num_of_subclasses variable.
---
src/hotspot/share/classfile/classFileParser.hpp
+ u2 parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
+ const u1* const
permitted_subclasses_attribute_start,
+ TRAPS);
Please fix indentation after copy'n'edit.
---
src/hotspot/share/oops/instanceKlass.cpp
247 if (classloader1 != classloader2) {
I'm not clear what rule this is verifying. The same module check
follows this one. The rule is that the subclass must be accessible to
the superclass implying:
1. same named module (regardless of class access modifiers); or
2. (implicitly in un-named module) same package if subclass not
public; or
3. public subclass
Having the same classloader implies same package, but that alone
doesn't address 2 or 3. So this doesn't conform to proposed JVMS rules.
264 if (_constants->tag_at(cp_index).is_klass()) {
265 Klass* k2 = _constants->klass_at(cp_index, CHECK_false);
You've copied this code from the nestmember checks but your changes
don't quite make sense to me. If we have already checked is_klass()
then klass_at() cannot lead to any
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Harold and David,
Just one comment.
We could save on the CSR's for:
make/data/jdwp/jdwp.spec
src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java
src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java
As we discussed with David, it could make sense to remove
duplication in the Serviceability class redefinition and
retransformation specs.
I'd suggest something like this webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/
If the direction looks okay then I could file RFE+CSR (and post an
RFR).
Thanks,
Serguei
On 5/18/20 22:26, David Holmes wrote:
Hi
Harold,
Adding serviceability-dev for the serviceability related changes.
Nit: "VM support for sealed classes"
This RFR covers the VM, core-libs, serviceability and even some
langtools tests. AFAICS only the javac changes are not included
here so when and where will they be reviewed and under what bug
id? Ideally there will be a single JBS issue for "Implementation
of JEP 360: Sealed types". It's okay to break up the RFRs across
different areas, but it should be done under one bug id.
Overall this looks good. I've looked at all files and mainly have
some style nits in various places. But there are some more
significant items below.
On 14/05/2020 7:09 am, Harold Seigel wrote:
Hi,
Please review this patch for JVM and Core-libs support for the
JEP 360 Sealed Classes preview feature. Code changes include
the following:
* Processing of the new PermittedSubclasses attribute to
enforce that
a class or interface, whose super is a sealed class or
interface,
must be listed in the super's PermittedSubclasses attribute.
* Disallow redefinition of a sealed class or interface if its
redefinition would change its PermittedSubclasses attribute.
* Support API's to determine if a class or interface is sealed
and, if
it's sealed, return a list of its permitted subclasses.
* asm support for the PermittedSubclasses attribute
I assume Remi is providing the upstream support in ASM? :) But
also see below ...
Open Webrev:
http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html
make/data/jdwp/jdwp.spec
There needs to be a sub-task and associated CSR request for this
JDWP spec update. I couldn't see this covered anywhere.
---
src/hotspot/share/classfile/classFileParser.cpp
3215 u2
ClassFileParser::parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
3216
const u1* const permitted_subclasses_attribute_start,
3217
TRAPS) {
Indention on L3216/17 needs fixing after the copy'n'edit.
3515 return _major_version == JVM_CLASSFILE_MAJOR_VERSION
&&
3516 _minor_version == JAVA_PREVIEW_MINOR_VERSION
&&
3517 Arguments::enable_preview();
Too much indentation on L3516/17
3790 // Check for PermittedSubclasses tag
That comment (copied from my nestmates code :) is in the wrong
place. It needs to be before
3788 if (tag == vmSymbols::tag_permitted_subclasses())
{
Minor nit: I would suggest checking
parsed_permitted_subclasses_attribute before checking ACC_FINAL.
3876 if (parsed_permitted_subclasses_attribute) {
3877 const u2 num_of_subclasses =
parse_classfile_permitted_subclasses_attribute(
3878 cfs,
3879 permitted_subclasses_attribute_start,
3880 CHECK);
Although it looks odd the preceding, similarly shaped, sections
all indent to the same absolute position. Can you make L3878/78/80
match please.
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
3884 sizeof(num_of_subclasses) + sizeof(u2) *
num_of_subclasses,
3885 "Wrong PermittedSubclasses
Re: RFR: JDK-8225056 VM support for sealed classes
- Mail original - > De: "David Holmes" > À: "Harold David Seigel" , "hotspot-runtime-dev" > , > "amber-dev" , "core-libs-dev" > , "serviceability-dev" > > Envoyé: Mardi 19 Mai 2020 07:26:38 > Objet: Re: RFR: JDK-8225056 VM support for sealed classes > Hi Harold, > > Adding serviceability-dev for the serviceability related changes. > > Nit: "VM support for sealed classes" > > This RFR covers the VM, core-libs, serviceability and even some > langtools tests. AFAICS only the javac changes are not included here so > when and where will they be reviewed and under what bug id? Ideally > there will be a single JBS issue for "Implementation of JEP 360: Sealed > types". It's okay to break up the RFRs across different areas, but it > should be done under one bug id. > > Overall this looks good. I've looked at all files and mainly have some > style nits in various places. But there are some more significant items > below. > > On 14/05/2020 7:09 am, Harold Seigel wrote: >> Hi, >> >> Please review this patch for JVM and Core-libs support for the JEP 360 >> Sealed Classes preview feature. Code changes include the following: >> >> * Processing of the new PermittedSubclasses attribute to enforce that >> a class or interface, whose super is a sealed class or interface, >> must be listed in the super's PermittedSubclasses attribute. >> * Disallow redefinition of a sealed class or interface if its >> redefinition would change its PermittedSubclasses attribute. >> * Support API's to determine if a class or interface is sealed and, if >> it's sealed, return a list of its permitted subclasses. >> * asm support for the PermittedSubclasses attribute > > I assume Remi is providing the upstream support in ASM? :) But also see > below ... Currently the version of ASM used JDK already supports sealed classes but with the attribute named PermittedSubtypes instead of PermittedSubclasses. This patch renames the attribute which is the right thing to do. Then when JDK 15 will be released, we will release ASM 9 with full support of PermittedSubclasses, with the right method names, docs, etc, that will be then integrated in JDK 16. So with my ASM hat, the changes to the 3 ASM classes are good. Rémi
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Harold,
Adding serviceability-dev for the serviceability related changes.
Nit: "VM support for sealed classes"
This RFR covers the VM, core-libs, serviceability and even some
langtools tests. AFAICS only the javac changes are not included here so
when and where will they be reviewed and under what bug id? Ideally
there will be a single JBS issue for "Implementation of JEP 360: Sealed
types". It's okay to break up the RFRs across different areas, but it
should be done under one bug id.
Overall this looks good. I've looked at all files and mainly have some
style nits in various places. But there are some more significant items
below.
On 14/05/2020 7:09 am, Harold Seigel wrote:
Hi,
Please review this patch for JVM and Core-libs support for the JEP 360
Sealed Classes preview feature. Code changes include the following:
* Processing of the new PermittedSubclasses attribute to enforce that
a class or interface, whose super is a sealed class or interface,
must be listed in the super's PermittedSubclasses attribute.
* Disallow redefinition of a sealed class or interface if its
redefinition would change its PermittedSubclasses attribute.
* Support API's to determine if a class or interface is sealed and, if
it's sealed, return a list of its permitted subclasses.
* asm support for the PermittedSubclasses attribute
I assume Remi is providing the upstream support in ASM? :) But also see
below ...
Open Webrev:
http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html
make/data/jdwp/jdwp.spec
There needs to be a sub-task and associated CSR request for this JDWP
spec update. I couldn't see this covered anywhere.
---
src/hotspot/share/classfile/classFileParser.cpp
3215 u2
ClassFileParser::parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
3216const
u1* const permitted_subclasses_attribute_start,
3217TRAPS) {
Indention on L3216/17 needs fixing after the copy'n'edit.
3515 return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
3516 _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
3517 Arguments::enable_preview();
Too much indentation on L3516/17
3790 // Check for PermittedSubclasses tag
That comment (copied from my nestmates code :) is in the wrong place. It
needs to be before
3788 if (tag == vmSymbols::tag_permitted_subclasses()) {
Minor nit: I would suggest checking
parsed_permitted_subclasses_attribute before checking ACC_FINAL.
3876 if (parsed_permitted_subclasses_attribute) {
3877 const u2 num_of_subclasses =
parse_classfile_permitted_subclasses_attribute(
3878cfs,
3879
permitted_subclasses_attribute_start,
3880CHECK);
Although it looks odd the preceding, similarly shaped, sections all
indent to the same absolute position. Can you make L3878/78/80 match please.
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
3884 sizeof(num_of_subclasses) + sizeof(u2) * num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class file
%s", CHECK);
Nits: please reformat as:
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
sizeof(num_of_subclasses) + sizeof(u2) * num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class file
%s", CHECK);
It would also look slightly better if you shortened the name of the
num_of_subclasses variable.
---
src/hotspot/share/classfile/classFileParser.hpp
+ u2 parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
+ const u1* const
permitted_subclasses_attribute_start,
+ TRAPS);
Please fix indentation after copy'n'edit.
---
src/hotspot/share/oops/instanceKlass.cpp
247 if (classloader1 != classloader2) {
I'm not clear what rule this is verifying. The same module check follows
this one. The rule is that the subclass must be accessible to the
superclass implying:
1. same named module (regardless of class access modifiers); or
2. (implicitly in un-named module) same package if subclass not public; or
3. public subclass
Having the same classloader implies same package, but that alone doesn't
address 2 or 3. So this doesn't conform to proposed JVMS rules.
264 if (_constants->tag_at(cp_index).is_klass()) {
265 Klass* k2 = _constants->klass_at(cp_index, CHECK_false);
You've copied this code from the nestmember checks but your changes
don't quite make sense to me. If we have already checked is_klass() then
klass_at() cannot lead to any exceptions.
272 if (name == k->name()) {
273 log_trace(class, sealed)("- Found it at
