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

[Kim Barrett]

> On Aug 21, 2018, at 11:11 AM, Hohensee, Paul <hohensee at amazon.com> 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 the new code arrangement.  And if we used anonymous
namespaces, I'd put these checker classes in such, in the only file
where they are used.

> 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.

Looks good to me, with these other changes Paul suggested.

> 
> Paul
> 
> On 8/21/18, 4:53 AM, "hotspot-gc-dev on behalf of Thomas Schatzl" <hotspot-gc-dev-bounces at openjdk.java.net on behalf of thomas.schatzl at oracle.com> wrote:
> 
>    Hi all,
> 
>      in the review for JDK-8208498 there has been a discussion about using
>    enums to indicate which types of regions can be added to a given
>    HeapRegionSet; looking at the code, ultimately this enum is only used
>    for verification, i.e. if the HeapRegionSet contains the right kind of
>    regions or not.
> 
>    Instead of adding random enum values every time you need a new
>    HeapRegionSet, I refactored the code so that the existing
>    HRSMtSafeChecker passed to HeapRegionSet instances also gets methods
>    for other verification, and implemented them.
> 
>    I also moved the various HRSMtSafeChecker instances to their point of
>    use in cpp files to imho reduce the clutter in hpp files a bit (i.e.
>    hide them completely in cpp files).
> 
>    CR:
>    https://bugs.openjdk.java.net/browse/JDK-8209700
>    Webrev:
>    http://cr.openjdk.java.net/~tschatzl/8209700/webrev/
>    Testing:
>    hs-tier1-3
> 
>    This change is based on JDK-8208498 that is also out for review.
> 
>    Thanks,
>      Thomas