Request for reviews (L): 6939207: refactor constant pool index processing

[John Rose]

On Apr 28, 2010, at 2:23 AM, Christian Thalinger wrote:

> On Fri, 2010-04-23 at 15:52 -0700, John Rose wrote:
>> On Apr 23, 2010, at 7:06 AM, Christian Thalinger wrote:
>> 
>>> I have to say I was a little confused about the get_index1,
>>> get_index2, ... naming as it looks like you were running out of method
>>> names and just append 1, 2, ... ;-)
>> 
>> You are right, it could look that way at first.  Can you suggest some
>> comments that would short-circuit that impression quickly?
> 
> Well, you could rename it to get_index_u{1,2,4} which would correspond
> to the other Bytes::get* functions.

Thanks, I like this idea.  I changed it to be this way.

One wrinkle: get_offset2 changes to get_offset_s2 (since it's signed).
I think it is good to advertise the signed-ness of the accessors.

> If you want to keep them maybe add a simple comment like:
> 
> // get_{index,offset,constant}{1,2,4} methods return an
> // index/offset/constant of the size of the appended number in bytes 
> //(1, 2, or 4 bytes).
> 
> You may rephrase for better English.
> 
> I just see that ciStreams.hpp has comments on most of these methods.
> 
>> 
>>> The change is very big.  I take a look at it again later.
>> 
>> That will be just fine.
> 
> src/share/vm/prims/jvmtiClassFileReconstituter.cpp:
> 
> Copyright year update missing.

Fixed.

> src/share/vm/prims/methodComparator.cpp:
> 
> -      if (_s_old->get_index_big() != _s_new->get_index_big())
> +      if (Bytes::get_Java_u2(_s_old->bcp() + 1) != Bytes::get_Java_u2(_s_new->bcp() + 1))
> 
> Why can't you use get_index2() here?

It's a pre-existing hack.  I added comments.  I thought about splitting up the comparisons, but didn't want to risk a bug.

    if (! _s_old->is_wide()) {
      // We could use get_index_u1 and get_constant_u1, but it's simpler to grab both bytes at once:
      if (Bytes::get_Java_u2(_s_old->bcp() + 1) != Bytes::get_Java_u2(_s_new->bcp() + 1))
        return false;
    } else {
      // We could use get_index_u2 and get_constant_u2, but it's simpler to grab all four bytes at once:
      if (Bytes::get_Java_u4(_s_old->bcp() + 1) != Bytes::get_Java_u4(_s_new->bcp() + 1))
        return false;
    }

> Otherwise looks good.

Thanks!

-- John
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20100501/5e02811f/attachment.html