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

Optimize any/all/noneSatisfy on UnifiedMapWithHashingStrategy - closes #1342 #1406

Merged
merged 1 commit into from
Oct 17, 2022

Conversation

Desislav-Petrov
Copy link
Contributor

@Desislav-Petrov Desislav-Petrov commented Oct 4, 2022

PR - would appreciate a review @donraab

Looks like existing tests already cover this refactoring

Copy link
Contributor

@donraab donraab left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution @Desislav-Petrov. I think it would be better to duplicate the functionality in the shortCircuit/shortCircuitWith methods than to externalize the functionality to MapIterate. MapIterate is intended to iterate over Maps, and this iterates over an array, requiring MapIterate to understand how the internals of UnifiedMap and UnifiedMapWithHashingStrategy are implemented.

@Desislav-Petrov
Copy link
Contributor Author

Thanks for the review @donraab - fair points, made the changes & rebased

@donraab donraab requested review from motlin and removed request for donraab October 11, 2022 02:06
Copy link
Contributor

@motlin motlin left a comment

Choose a reason for hiding this comment

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

To review this, I checked it out locally and diff'd UnifiedMap with UnifiedMapWithHashingStrategy. I found that the new methods were added in a different location, so diff'ing didn't work. I made the fix so that I could continue the review, but you'll have to apply it as well.

d4018e6

Once I did this I could see that the new implementation and the old are identical, and are good to go. Please move the methods to the same positions and then we ought to merge this.

@Desislav-Petrov
Copy link
Contributor Author

Thx for the feedback @motlin - changes made

@motlin
Copy link
Contributor

motlin commented Oct 14, 2022

@Desislav-Petrov could you also rebase onto upstream/master and then I'll merge?

@Desislav-Petrov
Copy link
Contributor Author

Done @motlin

@motlin motlin merged commit 73d6e96 into eclipse:master Oct 17, 2022
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.

3 participants