CRR: 6804436: G1: heap region indices should be size_t (S)

[Bengt Rutisson]

Tony,

I had a look at this as well since it is connected to the previous 
review: "CRR: 7042285 and 7045330 : fixes and improvements to the 
HeapRegionSeq class (S/M) ".

It looks good.

Two things, though:

* I think it is very closely connected to the previous review. So, close 
that I in fact think they should be reviewed and pushed together. I 
don't really see why they are separate changes.

* I still think it is a bit strange that G1_INVALID_HRS_INDEX is defined 
in heapRegion.hpp. I would prefer to keep it in heapRegionSeq. The only 
use in heapRegion is in the constructor. This constructor is only called 
from two places:
    - G1CollectedHeap::expand()
       Here we actually know the index we want and could pass that to 
the constructor. No need to first set it to an invalid value. I think 
this would be even cleaner with the get_heap_region()  method that I 
suggested in the previous review.
   - G1CollectedHeap::initialize()
        Here we know about heapRegionSeq and could ask it for the 
invalid value and pass that to the constructor.

Bengt

On 2011-06-02 00:18, Tony Printezis wrote:
> Stefan and John,
>
> Thanks! All set with this one too.
>
> Tony
>
> On 06/01/2011 03:11 PM, Stefan Karlsson wrote:
>> Looks good.
>>
>> StefanK
>>
>> On 06/01/2011 02:04 PM, Tony Printezis wrote:
>>> Hi,
>>>
>>> Could I please have a couple of code reviews for this simple change:
>>>
>>> http://cr.openjdk.java.net/~tonyp/6804436/webrev.0/
>>>
>>> Some of the int -> size_t changes were done as part of the 
>>> HeapRegionSeq cleanup (see the separate code review request for 
>>> 7042285 and 7045330 that I just sent out). This change does the rest 
>>> (specifically: the changes related to the HeapRegion class).
>>>
>>> Note: I know that the webrev index says that some files have more 
>>> than one change applied to them. This is incorrect and a side-effect 
>>> of generating the webrev from a workspace with multiple patches 
>>> stacked up. The diffs are actually correct (I checked).
>>>
>>> Tony
>>>
>>