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

[Christian Thalinger]

On Thu, 2010-04-22 at 00:49 -0700, John Rose wrote:
> > src/share/vm/ci/ciStreams.hpp:
> > 
> >   int get_index_giant() const {
> > -    assert_index_size(4);
> > +    assert_index_size(4); assert_native_index();
> >     return Bytes::get_native_u4(_bc_start+1);
> >   }
> > 
> > Should we assert on has_giant_index in this one?
> 
> That's what assert_index_size(4) does, right?

Indeed.  I didn't look what assert_index_size is actually doing, sorry.

> 
> > src/share/vm/interpreter/templateTable.hpp:
> > 
> > +                                      size_t index_size); // one of 1,2,4
> > 
> > What do you think about using enum values instead of sizeof(u?) in the
> > code?
> 
> That would provide slightly stronger type checking for those
> parameters.  (Size_t is a nondescript type.)  But we use tiny ints for
> mode flags in so many other places...  I thought about putting this in
> some place like bytes.hpp or globalDefinitions:
> 
>    enum UFieldSize { u1Size = 1, u2Size = 2, u4Size = 4 };
> 
> Then the "size_t index_size" parameters would change "UFieldSize
> index_size".  But it occurred to me that this will look inconsistent
> with the more generic definitions of wordSize, etc.  Is there a
> particular benefit that would come from doing it "extra specially
> right" just at this point?  I'm inclined not to do it.

Besides that you cannot pass a wrong size?  No.  Keep it as it is.

> In the meantime, I pushed the refactoring further towards consistent naming.  Tell me what you think of this version:
>   http://cr.openjdk.java.net/~jrose/6939207/webrev.02

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, ... ;-)

The change is very big.  I take a look at it again later.

-- Christian