Request for code review - JDK-8141123 Change int's to size_t in FreeIdSet

Alexander Harlap alexander.harlap at oracle.com
Mon Nov 16 18:25:01 UTC 2015


Here is another revision,

http://cr.openjdk.java.net/~aharlap/8141123/webrev.02/

It has more cleanup.

Version was tested by RBT.

Alex

On 11/12/2015 4:09 PM, Alexander Harlap wrote:
> Here is new revision of change:
>
> http://cr.openjdk.java.net/~aharlap/8141123/webrev.01/
>
> Alex
>
> On 11/12/2015 4:35 AM, Thomas Schatzl wrote:
>> Hi Alex,
>>
>> On Wed, 2015-11-11 at 14:55 -0500, Alexander Harlap wrote:
>>> Change int's to size_t in FreeIdSet
>>> JDK-8141123
>>>
>>> Proposed change:
>>>
>>> http://cr.openjdk.java.net/~aharlap/8141123/webrev.00
>>>
>> Some comments:
>>
>>    - could you use a current version of webrev that adds links to the
>> bottom of the page? That would improve the "review experience" a lot :)
>> I think the current one from
>> http://openjdk.java.net/guide/webrevHelp.html does that.
>>
>>    - please rename the _sz member to _size. Also, it should be an
>> unsigned type.
>>    - _index should be size_t too if I read the code correctly.
>>
>>    - the return value of claim_par_id() should also be uint.
>>
>>    - please add braces and newlines around one-line loops etc.
>>
>> E.g.
>>
>> 516     for (int j = 0; j < NSets; j++) _sets[j] = NULL;
>>
>> should be
>>
>> for (int j = 0; j < NSets; j++) {
>>    _sets[j] = NULL;
>> }
>>
>>   - I think the allocation kind of the _ids array should be mtGC, not
>> mtInternal. Only GC uses it.
>>
>>   - remove the FreeIdSetPtr typdef. It is not consistently used, and
>> actually there is only one use anyway.
>>
>>   - FreeIdSet::set_safepoint() seems to be never called. I think it
>> allows you to remove a *lot* of unused stuff.
>>
>>   - same with claim_perm_id().
>>
>>   - please remove all code ifdef'ed by FID_STATS. I doubt anyone ever
>> used it for a long time.
>>
>> That should be enough for now :)
>>
>> Thanks,
>>    Thomas
>>
>>
>




More information about the hotspot-gc-dev mailing list