review (M) for 4926272: methodOopDesc::method_from_bcp is unsafe

[Vladimir Kozlov]

I think it looks good. I would only suggest (as John) to keep swapped arguments (method, bcp) order if possible.

Thanks,
Vladimir

John Rose wrote:
> On Jan 11, 2011, at 2:31 PM, Tom Rodriguez wrote:
> 
>> http://cr.openjdk.java.net/~never/4926272
>> 4926272: methodOopDesc::method_from_bcp is unsafe
> 
> This is a painful but long-needed cleanup.  There is lots of good code deletion.  Thank you.
> 
> Bytecode_member_ref and Bytecode_loadconstant would look better as subclasses of Bytcode also.  Was there a mismatch somewhere?  Suggest pushing the Bytecode superclass down into those guys.
> 
> That's an economical move passing methodOop(NULL) from the CI.
> 
> The two binary overloadings of Bytecodes::code_at have swapped arg-order; this smells worse now.  Can something be done?  Maybe just put the (now required) methodOop first always or (better) second always?  Wait, I guess I see the convention:  bcp/oop vs. oop/bci.  (Seen in Bytecode constructors and various accessors in Bytecodes::.)  That is an uncomfortable way to deal with the bci/bcp duality, which might be worth fixing now.  Fixing it might double your (already considerable) work.  OTOH, fixing that might just touch use points you are already touching now anyway.  Suggest moving the methodOop after the bcp/bci in all touched API points and immediate neighbors.
> 
> The name Bytecode_invoke_at_check doesn't read so well anymore; it is not part of a pattern.  Suggest Bytecode_invoke_check.
> 
> Reviewed!
> 
> -- John