On 3/27/20 11:59 AM, Paul Sandoz wrote:
Hi Mandy,
Very thorough, bravo!
Thanks.
Minor suggestions below.
Paul.
MethodHandleNatives.java
—
142
143 /**
144 * Flags for Lookup.ClassOptions
145 */
146 static final int
147 NESTMATE_CLASS = 0x00000001,
148 HIDDEN_CLASS = 0x00000002,
149 STRONG_LOADER_LINK = 0x00000004,
150 ACCESS_VM_ANNOTATIONS = 0x00000008;
151 }
Suggest you add a comment to keep the values in sync with the VM component.
Already in the class spec of this Constants class. The values of all
constants defined in this Constants class are verified in sync with VM
(see verifyConstants).
MethodHandles.java
—
1786 * (Given the {@code Lookup} object returned this method, its
lookup class
1787 * is a {@code Class} object for which {@link Class#getName()}
returns a string
1788 * that is not a binary name.)
“
(The {@code Lookup} object returned from this method has a lookup class that is
a {@code Class} object whose {@link Class#getName()} returns a string
that is not a binary name.)
“
1902 Set<ClassOption> opts = options.length > 0 ? Set.of(options) :
Set.of();
You can just do:
Set<ClassOption> opts = Set.of(options)
And/or inline it into the subsequent method call. The implementation of Set.of
checks the array length.
Great to know. Thanks.
2001 ClassDefiner makeHiddenClassDefiner(byte[] bytes,
I think you can telescope the methods for non-name and name accepting since
IIUC the name is derived from the byte[]. Thereby you can remove some code
duplication. i.e. pull ClassDefiner.className out from ClassDefiner and place
the logic in the factory methods. Alternative push the factory methods into
ClassDefiner to keep all the logic together.
Ok. I will move the className out.
3797 public enum ClassOption {
Shuffle up to be closer to the defineHiddenClass
Moved before defineHiddenClass.
3798 /**
3799 * This class option specifies the hidden class be added to
3800 * {@linkplain Class#getNestHost nest} of a lookup class as
3801 * a nestmate.
Suggest:
"This class option specifies the hidden class “ -> “Specifies that a hidden
class
3812 * This class option specifies the hidden class to have a
<em>strong</em>
“Specifies that a hidden class have a …"
Specifies that a hidden class has a...
3813 * relationship with the class loader marked as its defining
loader,
3814 * as a normal class or interface has with its own defining
loader.
3815 * This means that the hidden class may be unloaded if and
only if
3816 * its defining loader is not reachable and thus may be
reclaimed
3817 * by a garbage collector (JLS 12.7).
StringConcatFactory.java
—
861 // use of @ForceInline no longer has any effect
?
Right, I should have explained this [1].
This @ForceInline is used by BytecodeStringBuilderStrategy that
generates code to have the same StringBuilder chain javac would emit. It
uses `@ForceInline` annotation which may probably be for performance.
It's believed people rarely uses this non-default strategy. This patch
changes StringConcatFactory to the standard defineHiddenClass method and
hence `@ForceInline` has no effect in the generated class for this
non-default strategy. If it turns out to be an issue, then we will
determine if it should enable the access to VM annotations (I doubt this
is supported strategy).
[1] https://bugs.openjdk.java.net/browse/JDK-8241548
