HI Michael,

Nice work, not easy this area!

It may be worth considering, as a separate issue, updating the example section 
in JavaDoc to be under @apiNote.

I have a mild preference to maintain the 80 char limit for JavaDoc, to me it’s 
easier to read. For code i don’t mind a 100 or more limit.

Add @since 9 to new public methods.


 470     /** Replace the last arrayLength parameter types with the component 
type of arrayType.
 471      * @param arrayType any array type
 472      * @param arrayLength the number of parameter types to change
 473      * @return the resulting type
 474      */

 475     /*non-public*/ MethodType asSpreaderType(Class<?> arrayType, int pos, 
int arrayLength) {

 476         assert(parameterCount() >= arrayLength);

 477         int spreadPos = pos;

Might as well adjust the JavaDoc for the new pos parameter (as you have done 
for asCollectorType).


 898      * @param spreadArgPos the position (zero-based index) in the argument 
list at which spreading should start.
 899      * @param arrayType usually {@code Object[]}, the type of the array 
argument from which to extract the spread arguments
 900      * @param arrayLength the number of arguments to spread from an 
incoming array argument
 901      * @return a new method handle which spreads an array argument at a 
given position,
 902      *         before calling the original method handle
 903      * @throws NullPointerException if {@code arrayType} is a null 
 904      * @throws IllegalArgumentException if {@code arrayType} is not an 
array type,
 905      *         or if target does not have at least
 906      *         {@code arrayLength} parameter types,
 907      *         or if {@code arrayLength} is negative,
 908      *         or if the resulting method handle's type would have
 909      *         <a href="MethodHandle.html#maxarity">too many parameters</a>

For the new asSpreader method, what are the constraints for spreadArgPos? What 
happens if < 0 or >= arrayLength for example?

Same applies to asCollector.

This is not in your patch but the assert in the following method is redundant 
(as reported by the IDE, it’s really handy sometimes apply the patch and view 
via diffs in the IDE), as DirectMethodHandle is not a subtype of 

MethodHandle viewAsType(MethodType newType, boolean strict) {
    // No actual conversions, just a new view of the same method.
    // Note that this operation must not produce a DirectMethodHandle,
    // because retyped DMHs, like any transformed MHs,
    // cannot be cracked into MethodHandleInfo.
    assert viewAsTypeChecks(newType, strict);
    BoundMethodHandle mh = rebind();
    assert(!((MethodHandle)mh instanceof DirectMethodHandle));
    return mh.copyWith(newType, mh.form);

 958         /**
 959          * Determines if a class can be accessed from the lookup context 
defined by this {@code Lookup} object. The
 960          * static initializer of the class is not run.
 961          *
 962          * @param targetClass the class to be accessed
 963          *
 964          * @return the class that has been accessed

Has it really been accessed by “accessClass” or just checked that it can be 

 965          *
 966          * @throws IllegalAccessException if the class is not accessible 
from the lookup class, using the allowed access
 967          * modes.
 968          * @exception SecurityException if a security manager is present 
and it
 969          *                              <a 
href="MethodHandles.Lookup.html#secmgr">refuses access</a>
 970          */
 971         public Class<?> accessClass(Class<?> targetClass) throws 
IllegalAccessException {

Great use of streams in MethodHandles.loop and good correlation with the 
specified steps in the JavaDoc to that in the code. Really clear and well 

3458      * The loop handle's result type is the same as the sole loop 
variable's, i.e., the result type of {@code init}.

s/variable’s/variable ?

3488      * // assume MH_initZip, MH_zipPred, and MH_zipStep are handles to the 
above methods
3489      * MethodHandle loop = MethodHandles.doWhileLoop(MH_initZip, 
MH_zipPred, MH_zipStep);
3490      * List<String> a = Arrays.asList(new String[]{"a", "b", "c", "d"});
3491      * List<String> b = Arrays.asList(new String[]{"e", "f", "g", "h"});
3492      * List<String> zipped = Arrays.asList(new String[]{"a", "e", "b", 
"f", "c", "g", "d", "h"});
3493      * assertEquals(zipped, (List<String>) loop.invoke(a.iterator(), 
3494      * }</pre></blockquote>

You don’t need to create an explicit array and pass that into Arrays.asList. 
Same for other related methods.


1720     static MethodHandle makeLoop(Class<?> tloop, List<Class<?>> targs, 
List<Class<?>> tvars, List<MethodHandle> init,
1721                                  List<MethodHandle> step, 
List<MethodHandle> pred, List<MethodHandle> fini) {
1722         MethodHandle[] ainit = 
1723         MethodHandle[] astep = 
1724         MethodHandle[] apred = 
1725         MethodHandle[] afini = 
1751     static List<MethodHandle> toArrayArgs(List<MethodHandle> hs) {
1752         return hs.stream().map(h -> h.asSpreader(Object[].class, 
1753     }

Can you use Stream.toArray(MethodHandle[]::new) rather than creating a List and 
then an array from that? (or expand to a lambda expression if you want to reuse 
MH_ARRAY_SENTINEL for an empty array).

I guess in the future there may be ample opporunity to specialize the generic 
“loop” mechanism with LambdaForms?


> On 13 Nov 2015, at 17:39, Michael Haupt <michael.ha...@oracle.com> wrote:
> Dear all,
> please review this change.
> RFE: https://bugs.openjdk.java.net/browse/JDK-8139885
> Corresponding JEP: https://bugs.openjdk.java.net/browse/JDK-8130227
> Webrev: http://cr.openjdk.java.net/~mhaupt/8139885/webrev.00/
> Thanks,
> Michael
> --
> Dr. Michael Haupt | Principal Member of Technical Staff
> Phone: +49 331 200 7277 | Fax: +49 331 200 7561
> Oracle Java Platform Group | LangTools Team | Nashorn
> Oracle Deutschland B.V. & Co. KG, Schiffbauergasse 14 | 14467 Potsdam, Germany
>       Oracle is committed to developing practices and products that help 
> protect the environment
> _______________________________________________
> mlvm-dev mailing list
> mlvm-dev@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Attachment: signature.asc
Description: Message signed with OpenPGP using GPGMail

mlvm-dev mailing list

Reply via email to