RFR: 8257912: Convert enum iteration to use range-based for loops
Ioi Lam
iklam at openjdk.java.net
Wed Dec 9 22:06:35 UTC 2020
On Wed, 9 Dec 2020 19:45:12 GMT, Gerard Ziemski <gziemski at openjdk.org> wrote:
>> * For JDK 17*
>>
>> Now that [range-based for loops](https://github.com/openjdk/jdk/pull/1488) are allowed by the [HotSpot Style Guide](https://github.com/openjdk/jdk/blob/52ab72127ddc3ca6575d9d58503ec39c5dff7ab1/doc/hotspot-style.md), I have converted the more verbose syntax:
>>
>> for (vmIntrinsicsIterator it = vmIntrinsicsRange.begin(); it != vmIntrinsicsRange.end(); ++it) {
>> vmIntrinsicID index = *it;
>> nt[as_int(index)] = string;
>> }
>>
>> to
>>
>> for (vmIntrinsicID index : EnumRange<vmIntrinsicID>{}) {
>> nt[as_int(index)] = string;
>> }
>>
>> I also removed the "convenient" declarations such as `vmIntrinsicsRange` and `vmIntrinsicsIterator` -- these are useful only when writing a traditional for loop, and are probably confusing to everyone else. I've left examples in enumIterator.hpp in case anyone wants to write a traditional for loop.
>>
>> I also added gtest cases for using range-based for loops with `EnumRange<>`.
>
> Marked as reviewed by gziemski (Committer).
> I like the change very much.
>
> Did you consider leaving the convenience symbol "vmSymbolsIterator" as is and then using it in the loop, so instead of:
>
> `for (vmSymbolID index : EnumRange<vmSymbolID>{}) {`
>
> we would have:
>
> `for (vmSymbolID index : vmSymbolsIterator) {`
>
> which would be more concise (but less self-documenting I guess) and more similar to:
>
> `for (JVMFlagOrigin origin : range) {`
Hi Gerard, thanks for the review.
`vmSymbolsIterator` is the "cursor" type. I think you meant this:
for (vmSymbolID index : vmSymbolsRange{}) {...}
which is probably less readable, because you have to find out what `vmSymbolsRange` actually is.
I think the following pattern is easier to read. It will also make the iteration of different enum types more consistent. Iteration of any enum type *E* can be done the same way with EnumRange<*E*>. You don't need to invent a new *E*Range type.
for (MyEnumType e : EnumRange<MyEnumType>{}) {...}`
Note that the `{}` is required here because we need to construct an instance of `EnumRange<MyEnumType>` with its no-arg constructor. You can also use `()` but I think that would be less readable -- it's not clear whether you are calling a function or constructing an object. With `{}`, it's clear that we are making an [initialization](https://en.cppreference.com/w/cpp/language/initialization) with the brace operator.
You can also initialize a non-default range with `EnumRange<MyEnumType>{start, end}`, etc.
-------------
PR: https://git.openjdk.java.net/jdk/pull/1707
More information about the hotspot-dev
mailing list