RFR: 8253402: Convert vmSymbols::SID to enum class [v4]

[Kim Barrett]

On Sun, 11 Oct 2020 22:54:21 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> Convert `vmSymbols::SID` to an `enum class` to provide better type safety.
>> 
>> - The original enum type `vmSymbols::SID` cannot be forward-declared. I moved it out of the `vmSymbols` class and
>>   renamed, so now it can be forward-declared as `enum class vmSymbolID : int;`, without including the large vmSymbols.hpp
>>   file.
>>   - This also breaks  the mutual dependency between the `vmSymbols` and `vmIntrinsics` classes. Now  the declaration of
>>     `vmIntrinsics` can be moved from vmSymbols.hpp to vmIntrinsics.hpp, where it naturally belongs.
>> - Type-safe enumeration (contributed by Kim Barrett)
>> for (vmSymbolsIterator it = vmSymbolsRange.begin(); it != vmSymbolsRange.end(); ++it) {
>>  vmSymbolID index = *it; ....
>> }
>> - I moved `vmSymbols::_symbols[]` to `Symbol::_vm_symbols[]`, and made it accessible via `Symbol::vm_symbol_at()`. This
>>   way, header files (e.g. fieldInfo.hpp) that need to convert from `vmSymbolID` to `Symbol*` don't need to include the
>>   large vmSymbols.hpp file.
>> - I changed the `VM_SYMBOL_ENUM_NAME` macro so that the users don't need to explicitly add the `vmSymbolID::` scope.
>> - I removed many unnecessary casts between `int` and `vmSymbolID`.
>> - The remaining casts are done via `vmSymbol::as_int()` and `vmSymbols::as_SID()` with range checks.
>> 
>> -----
>> If this is successful, I will do the same for `vmIntrinsics::ID`.
>
> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
> 
>   added missing #include <limits> from enumIterator.hpp

Changes requested by kbarrett (Reviewer).

src/hotspot/share/classfile/vmSymbols.hpp line 714:

> 712:   }
> 713:   static constexpr bool is_valid_id(vmSymbolID sid) {
> 714:     return (static_cast<int>(sid) >= FIRST_SID && static_cast<int>(sid) < SID_LIMIT);

Why not just `return is_valid_id(static_cast<int>(sid));`

src/hotspot/share/classfile/vmSymbols.hpp line 728:

> 726:
> 727:   static constexpr int number_of_symbols() {
> 728:     return SID_LIMIT;

SID_LIMIT includes NO_SID, so this seems to be off-by-one.

src/hotspot/share/classfile/vmSymbols.hpp line 732:

> 730:
> 731:   enum {
> 732:     log2_SID_LIMIT = 11         // checked by an assert at start-up

[pre-existing, could be followup]
Why isn't this checked with a static assert right here?
`SID_LIMIT <= (1 << log2_SID_LIMIT)`
is (and always has been) compile-time checkable. And log2_SID_LIMIT should no longer be an enum. (Note that the value
isn't currently constexpr-calculable because round_up_power_of_2 can't currently be constexpr.)

src/hotspot/share/utilities/enumIterator.hpp line 91:

> 89:
> 90: template<typename T>
> 91: class EnumIterationTraits : AllStatic {

A comment for this class might be helpful. Something like "A helper class for EnumIterator, computing some additional
information the iterator uses, based on T and EnumeratorRange." The main point being that users of enum iteration don't
need to think about this.

src/hotspot/share/utilities/enumIterator.hpp line 56:

> 54: //
> 55: //
> 56: // EnumIterationTraits -- defines the static range of all possible values of the enum.

This traits class isn't really interesting to users of this facility; it's more of an implementation detail.  See my
comment below on the definition.  It's EnumeratorRange and (especially) ENUMERATOR_RANGE that are interesting to users.

src/hotspot/share/utilities/enumIterator.hpp line 138:

> 136:   }
> 137:
> 138:   // True if the enumerators designate the same value.

"True if the iterators designate the same enumeration value."  (Sorry about that.)  Similarly below for operator!=.

-------------

PR: https://git.openjdk.java.net/jdk/pull/276