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

[Tony Printezis]

On 06/08/2011 04:10 AM, Bengt Rutisson wrote:
>
> 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.

Yeah, you're right. I did a bunch of the int -> size_t changes as part 
of the first changeset (as I didn't think there was much point in 
rewriting half the HeapRegionSeq class using the wrong type and fixing 
it later). And, when I went it to do the rest it turned out there was 
not that many additional changes. When I push I'll fold both changesets 
into one and tag it with all three CRs.

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

Yeah, I was not crazy about having another "invalid" index. But maybe 
it's worth it. OK, I'll change that too and I'll post the new set of 
changes in a bit.

Tony

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