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

[Hohensee, Paul]

Lgtm,

Paul

On 8/22/18, 1:27 AM, "Thomas Schatzl" <thomas.schatzl at oracle.com> wrote:

    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