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