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

[Ioi Lam]

On Sun, 27 Sep 2020 11:09:30 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

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

Hi Kim, I have integrated the 2-style enumerator that you sent me off-line. Usage info (see enumIterator.hpp for
details).

// Example (see vmSymbols.hpp/cpp)
//
// ENUMERATOR_RANGE(vmSymbolID, vmSymbolID::FIRST_SID, vmSymbolID::LAST_SID)
// constexpr EnumRange<vmSymbolID> vmSymbolsRange;
// using vmSymbolsIterator = EnumIterator<vmSymbolID>;
//
// /* Without range-based for, allowed */
// for (vmSymbolsIterator it = vmSymbolsRange.begin(); it != vmSymbolsRange.end(); ++it) {
//  vmSymbolID index = *it; ....
// }
//
// /* With range-base for, not allowed by HotSpot coding style yet */
// for (vmSymbolID index : vmSymbolsRange) {
//    ....
// }

I have rewritten all the iteration loops using the "Without range-based for" style. We can change to the "With
range-base for" style when the HotSpot Coding Style Guide allows it.

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

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