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

OAK-11360: remove usage of Guava Ints.contains() #1958

Merged
merged 2 commits into from
Jan 8, 2025
Merged

OAK-11360: remove usage of Guava Ints.contains() #1958

merged 2 commits into from
Jan 8, 2025

Conversation

reschke
Copy link
Contributor

@reschke reschke commented Jan 8, 2025

No description provided.

@reschke reschke requested review from mbaedke and rishabhdaim January 8, 2025 08:50
@reschke reschke self-assigned this Jan 8, 2025
Copy link

github-actions bot commented Jan 8, 2025

Commit-Check ✔️

Copy link
Contributor

@Amoratinos Amoratinos left a comment

Choose a reason for hiding this comment

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

LGTM

}

public static boolean canCreateTypedField(Type<?> type) {
return Ints.contains(TYPABLE_TAGS, type.tag());
return TYPABLE_TAGS.contains(type.tag());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return TYPABLE_TAGS.contains(type.tag());
return Arrays.stream(TYPABLE_TAGS).anyMatch(tag -> tag == type.tag());

and we keep the type of TYPABLE_TAGS as int[].

Copy link
Contributor

Choose a reason for hiding this comment

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

Or we could change TYPABLE_TAGS to Set and take advantage of Set.contains API.

@reschke wdyt ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to avoid to convert ints to Integers every time we do the check.

I also assumed that the original author believed that sorting the array/list would help improve performance.

If this is not the case we of course could make it a Set.

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK List.contains always run in linear time irrespective of whether List is sorted or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sorting only helps if the lower values need to be looked up more frequently.

We could check the code what is really needed here. Or we can just keep things the way they are :-)

@reschke reschke merged commit bcb6527 into trunk Jan 8, 2025
5 of 6 checks passed
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.

4 participants