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

[1.x] Use hashCode instead of XOR hash in extraHash computation #1405

Merged
merged 2 commits into from
Sep 27, 2024

Conversation

Friendseeker
Copy link
Member

@Friendseeker Friendseeker commented Sep 23, 2024

Issue

From #1399

We use parentsHashes.fold(currentExtraHash)(_ ^ _) which is a bad way to combine hashes together. If we use XOR to combine different hashes, identical hashes gets cancelled out. Since we are doing hierarchical hashing here I have a concern that such cancellation would happen.

Also nowhere else in Zinc codebase do we use XOR to combine hashes, so there is also a consistency issue.

Fix

Replace XOR with hashCode.

@Friendseeker Friendseeker requested review from eed3si9n, dwijnand and BillyAutrey and removed request for dwijnand, BillyAutrey and eed3si9n September 23, 2024 04:09
@Friendseeker Friendseeker marked this pull request as draft September 23, 2024 04:20
@Friendseeker
Copy link
Member Author

Friendseeker commented Sep 23, 2024

I guess this is not a trivial change after all... CIs are all failing

Shall investigate further

@Friendseeker Friendseeker marked this pull request as ready for review September 23, 2024 04:30
@Friendseeker Friendseeker changed the title [1.10.x] Use hashCode instead of XOR hash in extraHash computation [1.x] Use hashCode instead of XOR hash in extraHash computation Sep 24, 2024
@eed3si9n eed3si9n merged commit 4d4c4ac into sbt:1.10.x Sep 27, 2024
8 checks passed
@Friendseeker Friendseeker deleted the remove-xor-hash-1 branch September 27, 2024 18:45
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.

2 participants