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

[Kim Barrett]

On Fri, 25 Sep 2020 22:23:34 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 (vmSymbolID index : vmSymbolsIterator()) {
>>    vm_symbol_index[as_int(index)] = index;
>> }
>> - 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 with a new target base due to a merge or a rebase. The incremental webrev excludes
> the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last
> revision:
>  - more vmEnums.hpp fixes; fixed minimal VM build
>  - Merge branch 'master' into 8253402-convert-vmsymbols-sid-to-enum-class
>  - Moved forward declaration of vmSymbolID to vmEnums.hpp
>  - clean up whitespaces and removed useless comment
>  - removed unnecessary include
>  - 8253402: Convert vmSymbols::SID to enum class

Changes requested by kbarrett (Reviewer).

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

> 1: /*

I think there are problems with this EnumIterator class. It was based in part on a prototype of mine, which I've been
revisiting and revising, because I think it has problems. That prototype, in turn, was based on the existing
WeakProcessorPhases::Iterator, which I think has some of the same failings. And I think this version expands on some of
those problems. I don't yet have a full review, but below are some observations and issues.  I'm working on an
alternative.

A fundamental question is what style of "iterator" do we want.

(1) One style is self-contained; you create a single iterator which knows both the current position and the iteration
limit, and step until a predicate (is_end() in the current code) is true.

(2) Another style is to have a pair of iterators, one designating the current position and the other designating the
iteration limit.  This is the style used by the C++ Standard Library.

Both my earlier prototype and the EnumIterator in this PR are 1-style but attempt (not necessarily very well, in my
opinion) to also provide 2-style behavior. The point of that currently seems to be to support the new "range-based for"
feature. (Said feature is currently not in the permitted list according to the Style Guide. I intentionally left it out
because I think its utility is pretty strongly dependent on adopting 2-style iterators, which is not very well
motivated without using the Standard Library.)

One requirement for an enum iterator (for me) is that it doesn't require a "fake" enumerator that designates the
exclusive end of the range.  The current proposal fails that test.

A problem with all of the variants is that they are trying to be both 1-style (providing is_end) and 2-style (providing
being/end), with the result that they do neither well. This is especially true for the variant in the PR. I think part
of the problem is that the begin/end functions don't belong to the iterator class; they should be part of a separate
range class.

Aside: I think it is possible to provide iteration that doesn't assume sequential enumerators if one is willing to have
some code duplication or has an enum that is x-macro based. While possibly an interesting exercise, I doubt that's
worth pursuing. Just mentioning it in case anyone thinks this would actually useful.

I'm not certain how to proceed. Maybe this should be moved elsewhere as not yet ready to be a widely used "utility"? Or
maybe go ahead with it with the intention of improving it?

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

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