Paul, thanks for review.

On 7/16/14 12:34 PM, Paul Sandoz wrote:

On Jul 14, 2014, at 4:04 PM, Vladimir Ivanov <vladimir.x.iva...@oracle.com> 
wrote:

http://cr.openjdk.java.net/~vlivanov/8050166/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8050166

Get rid of the following methods in j.l.i.MethodHandle:
  * convertArguments(MethodType newType)
  * bindArgument(int pos, BasicType basicType, Object value)
  * bindReceiver(Object receiver)
  * dropArguments(MethodType srcType, int pos, int drops)
  * permuteArguments(MethodType newType, int[] reorder)

Testing: jdk/java/lang/invoke, jdk/java/util/streams, nashorn, octane w/ "-ea 
-esa" and COMPILE_THRESHOLD={0,30}.


Looks good. Minor points, take it or leave it:

MethodHandles. permuteArgumentChecks

Might be more preferable for permuteArgumentChecks to return true or false and do the 
"throw newIllegalArgumentException" on a false return. Then if would play 
better with it's use in the assert, since it can throw an assertion error.

   assert target == originalTarget;
   assert permuteArgumentChecks(reorder, newType, target.type()) : "bad reorder 
array: "+Arrays.toString(reorder)
It's incorrect to split the assert. It's either target & reorder are unchanged or new values pass all checks. And if exception is thrown outside permuteArgumentChecks, we lose detailed error message.

So, I'd leave this code as is.

MethodHandles.insertArgumentPrimitive

Might be more consistent to always use a return value in the switch:

2258         switch (w) {
2259         default:      intValue = ValueConversions.widenSubword(value);    
break;
2260         case INT:     intValue = (int)value;                              
break;

default: return result.bindArgumentI(pos, ValueConversions.widenSubword(value));
case INT: return result.bindArgumentI(pos, (int)value);
Agree. Fixed.

Best regards,
Vladimir Ivanov
_______________________________________________
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Reply via email to