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
>>
>>
>>
