Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8257466: Improve enum iteration #1530

Closed
wants to merge 3 commits into from

Conversation

kimbarrett
Copy link

@kimbarrett kimbarrett commented Dec 1, 2020

Please review this collection of improvements to the recently added enum
iteration facility. These improvements are based on usage in some
in-development changes.

(1) Added EnumType nested type to EnumRange and EnumIterator. It is an alias
for the enum type that is a template parameter for those classes. This is
useful when dealing with a range or iterator whose associated enum type is
not known because the range/iterator is a template parameter.

(2) Added EnumRange::index(T), which converts the argument enumerator to
a zero-based index into the range of values. This is useful when mapping
from an enumerator to a corresponding array index, for example.

(3) Allow enum range bounds to be specified using start and (exclusive) end
integral (underlying type) values. This is useful when dealing with enums
that are just value ranges and don't have named enumerators. As part of
this, changes the enum iteration traits mechanism to use start/end rather
than first/last, as that seems to be a little easier to deal with.

(4) Added accessors for the first and last enumerator values of a range.

(5) Added gtest for enum iteration.

Testing: tier1

/label hotspot


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1530/head:pull/1530
$ git checkout pull/1530

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 1, 2020

👋 Welcome back kbarrett! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label Dec 1, 2020
@openjdk
Copy link

openjdk bot commented Dec 1, 2020

@kimbarrett
The hotspot label was successfully added.

@kimbarrett kimbarrett marked this pull request as ready for review Dec 1, 2020
@openjdk openjdk bot added the rfr Pull request is ready for review label Dec 1, 2020
@mlbridge
Copy link

mlbridge bot commented Dec 1, 2020

Webrevs

iklam
iklam approved these changes Dec 1, 2020
template<> struct EnumeratorRange<T> { \
static constexpr EnumeratorRangeImpl::Underlying<T> _start = \
EnumeratorRangeImpl::underlying<T>(Start); \
static constexpr EnumeratorRangeImpl::Underlying<T> _end = \
Copy link
Member

@iklam iklam Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to call underlying<T>(Start) here? Can it be simplified as

static constexpr EnumeratorRangeImpl::Underlying<T> _start = Start;

or

static constexpr EnumeratorRangeImpl::Underlying<T> _start {Start};

Copy link
Author

@kimbarrett kimbarrett Dec 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd earlier considered requiring the type of Start and End be Underlying rather than convertible to that. The underlying() function is a partial remnant of that. I'll remove it.

Copy link
Author

@kimbarrett kimbarrett Dec 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using brace initialization is the way to go, as it prevents narrowing.

@openjdk
Copy link

openjdk bot commented Dec 1, 2020

@kimbarrett This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8257466: Improve enum iteration

Improve support for iteration on enums that are just range of values, without named enumerators.

Reviewed-by: iklam, lfoltan

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Dec 1, 2020
iklam
iklam approved these changes Dec 2, 2020
lfoltan
lfoltan approved these changes Dec 2, 2020
Copy link
Member

@lfoltan lfoltan left a comment

Changes looks good Kim.
Lois

@kimbarrett
Copy link
Author

kimbarrett commented Dec 3, 2020

Thanks for reviewing @iklam and @lfoltan

@kimbarrett
Copy link
Author

kimbarrett commented Dec 3, 2020

/summary Improve support for enums that are just range of values, without named enumerators.

@openjdk
Copy link

openjdk bot commented Dec 3, 2020

@kimbarrett Setting summary to Improve support for enums that are just range of values, without named enumerators.

@kimbarrett
Copy link
Author

kimbarrett commented Dec 3, 2020

/summary Improve support for iteration on enums that are just range of values, without named enumerators.

@openjdk
Copy link

openjdk bot commented Dec 3, 2020

@kimbarrett Updating existing summary to Improve support for iteration on enums that are just range of values, without named enumerators.

@kimbarrett
Copy link
Author

kimbarrett commented Dec 3, 2020

/integrate

@openjdk openjdk bot closed this Dec 3, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 3, 2020
@openjdk
Copy link

openjdk bot commented Dec 3, 2020

@kimbarrett Pushed as commit 3932527.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@kimbarrett kimbarrett deleted the improve_enum_iter branch Dec 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated
3 participants