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

[Tony Printezis]

Bengt,

Here's the new webrev that includes all the changes (I'll push them as a 
single changeset):

http://cr.openjdk.java.net/~tonyp/7045330.6804436.7042285/webrev.0/webrev.all/

And here are my latest the changes wrt to the version you reviewed:

http://cr.openjdk.java.net/~tonyp/7045330.6804436.7042285/webrev.0/webrev.2.G1HRSLatest/

Let me know if you think I haven't addressed your concerns.

Tony

On 06/08/2011 10:43 AM, Tony Printezis wrote:
>
>
> 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
>>>>>
>>>>
>>