On Tue, 3 Nov 2020 13:45:57 GMT, Kim Barrett <kbarr...@openjdk.org> wrote:
>> Coleen Phillimore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> More review comments from Stefan and ErikO
>
> src/hotspot/share/gc/shared/weakProcessorPhases.hpp line 50:
>
>> 48: };
>> 49:
>> 50: typedef uint WeakProcessorPhase;
>
> This was originally written with the idea that WeakProcessorPhases::Phase
> (and WeakProcessorPhase) should be a scoped enum (but we didn't have that
> feature yet). It's possible there are places that don't cope with a scoped
> enum, since that feature wasn't available when the code was written, so there
> might have be mistakes.
>
> But because of that, I'd prefer to keep the WeakProcessorPhases::Phase type
> and the existing definition of WeakProcessorPhase. Except this proposed
> change is breaking that at least here:
>
> src/hotspot/share/gc/shared/weakProcessor.inline.hpp
> 116 uint oopstorage_index = WeakProcessorPhases::oopstorage_index(phase);
> 117 StorageState* cur_state = _storage_states.par_state(oopstorage_index);
> =>
> 103 StorageState* cur_state = _storage_states.par_state(phase);
>
> I think eventually (as in some future RFE) this could all be collapsed to
> something provided by OopStorageSet.
> enum class : uint WeakProcessorPhase {};
>
> ENUMERATOR_RANGE(WeakProcessorPhase,
> static_cast<WeakProcessorPhase>(0),
> static_cast<WeakProcessorPhase>(OopStorageSet::weak_count));
> and replacing all uses of WeakProcessorPhases::Iterator with
> EnumIterator<WeakProcessorPhase> (which involves more than a type alias).
>
> Though it might be possible to go even further and eliminate
> WeakProcessorPhases as a thing separate from OopStorageSet.
Ok, so I'm not sure what to do with this:
enum Phase {
// Serial phase.
JVMTI_ONLY(jvmti)
// Additional implicit phase values follow for oopstorages.
`};`
I've removed the only thing in this enum.
-------------
PR: https://git.openjdk.java.net/jdk/pull/967
