Request for code review - JDK-8141123 Change int's to size_t in FreeIdSet
Thomas Schatzl
thomas.schatzl at oracle.com
Thu Nov 12 09:35:20 UTC 2015
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