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