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

Decommission Enum.values() and replace it with Enum.entries #284

Merged
merged 7 commits into from
Aug 2, 2022

Conversation

qwwdfsad
Copy link
Contributor

No description provided.

@qwwdfsad qwwdfsad marked this pull request as ready for review December 14, 2021 15:35
@qwwdfsad qwwdfsad requested a review from elizarov December 14, 2021 15:35
@CLOVIS-AI
Copy link

What are the benefits of EnumEntries<E> being a List<E> instead of a Set<E>? As an immutable set, it could reuse the same strategies as Java's EnumMap for a fast contains() implementation.

@gildor
Copy link
Contributor

gildor commented Jun 16, 2022

@CLOVIS-AI Set is an unordered collection, but the order is very important for Enum values in many cases, also you cannot access values by index (ordinal) in set, which is one more important case for collection of enum values

@elizarov
Copy link
Contributor

@CLOVIS-AI What are the benefits of EnumEntries<E> being a List<E> instead of a Set<E>? As an immutable set, it could reuse the same strategies as Java's EnumMap for a fast contains() implementation.

To add to the answer by @gildor, having EnumEntries<E> as List<E> does not prevent us from reusing the strategy for a fast contains() implementation, but it also makes possible for a fast retrieval of an entry by its ordinal (index) using get(Int) method which a Set does not have.

@qwwdfsad qwwdfsad requested review from ilya-g and elizarov July 25, 2022 14:17
Copy link
Member

@ilya-g ilya-g left a comment

Choose a reason for hiding this comment

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

Overall good text.
Also, since it proposes to introduce new API in a new package, it may make sense to explain that choice: why new package, and why it is named like that.

@qwwdfsad qwwdfsad requested review from elizarov and ilya-g August 1, 2022 16:19
@qwwdfsad qwwdfsad merged commit 435606e into master Aug 2, 2022
@qwwdfsad qwwdfsad deleted the enum-entries branch August 2, 2022 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants