RFR (S): 8209700: Remove HeapRegionSetBase::RegionSetKind for a more flexible approach

Thomas Schatzl thomas.schatzl at oracle.com
Wed Aug 22 08:26:26 UTC 2018


Hi Paul,

  thanks for your review.

On Tue, 2018-08-21 at 15:11 +0000, Hohensee, Paul wrote:
> I'd probably keep the HeapRegionSetChecker subclass
> declarations/definitions in heapRegionSet.* rather than moving them
> to g1CollectedHeap.cpp and heapRegionManager.cpp. heapRegionSet.* is
> where the base class is defined, and future refactoring might require
> moving them out of g1CollectedHeap.cpp and heapRegionManager.cpp. If
> they stay in heapRegionSet.*, that'd be unnecessary. But, it's your
> call.

I prefer to keep such things in the only file they are used,
particularly if it is a cpp file to hide stuff and reduce hpp file
contents. Although it does not help with ODR  as seen recently as a cpp
file does not automatically have it's own local namespace... (duh)

> In heapRegionManager.cpp:
> 
> #include "heapRegionManager.hpp"
> =>
> #include "gc/g1/heapRegionManager.hpp"
> 
> This #include isn't strictly necessary given that there already is
> 
> #include "gc/g1/heapRegionManager.inline.hpp"
> 
> In heapRegionSet.hpp:
> 
> Since you've replaced _mt_safety_checker with _verifier, you might
> want to change the corresponding formal parameter names from
> mt_safety_checker to verifier, e.g., in the HeapRegionSetBase,
> HeapRegionSet, and FreeRegionList constructors.

Fixed.

http://cr.openjdk.java.net/~tschatzl/8209700/webrev.0_to_1/ (diff)
http://cr.openjdk.java.net/~tschatzl/8209700/webrev.1/ (full)

Thomas




More information about the hotspot-gc-dev mailing list