David, On May 16, 2013, at 7:12 AM, David Holmes wrote:
> Hi Rickard, > > On 15/05/2013 7:37 PM, Rickard Bäckman wrote: >> can I please have this change reviewed? >> >> My interpretation is that this isn't really a bug, since the parameter >> sig_type is never set to [. > > Right - it looks like a bug visually but turns out to be dead code. FYI the > "sig_type == '['" was added to the if under 4639363 - no idea why. I don't > see [ ever being used for sigtype. +1 > >> The suggested change is to remove the check for [ in the if and add an >> assert. I also created a boolean >> to track handle creation to simplify cleanup. > > Not sure the boolean was worth the effort - the existing check for L > suffices. :) Sure, but I like not repeating checks :) > > Even the assert seems overly conservative given the single caller :) At least it is safe. > > Still trying to decide what was meant in the bug report about the lack of an > "enclosing handle block" ?? My guess was that we have tried to do { make_local() destroy_local() } I don't think this makes things clearer in this case. > > Good to go if it hasn't already :) Thank you for the review. (Too late to make it to the list though :) ) /R > > Cheers, > David > > >> >> The caller of this method is InterpreterRuntime::post_field_modification >> which sets the sig_type to: >> >> switch(cp_entry->flag_state()) { >> case btos: sig_type = 'Z'; break; >> case ctos: sig_type = 'C'; break; >> case stos: sig_type = 'S'; break; >> case itos: sig_type = 'I'; break; >> case ftos: sig_type = 'F'; break; >> case atos: sig_type = 'L'; break; >> case ltos: sig_type = 'J'; break; >> case dtos: sig_type = 'D'; break; >> default: ShouldNotReachHere(); return; >> } >> >> Testing done: nsk.jvmti.testlist with fastdebug build. >> >> Webrev: http://cr.openjdk.java.net/~rbackman/4965252/ >> >> Thanks >> /R >> >> >>