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