Skip to content

[sil] Add missing flag to SILDeclRef's hash and change SILDeclRef to use llvm::hash_combine instead of rolling its own hash combiner. #79825

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

Conversation

gottesmm
Copy link
Contributor

@gottesmm gottesmm commented Mar 6, 2025

This is really old code from before llvm::hash_combine existed. We really shouldn't be rolling out own hash combine when we have something that we are consistently using from LLVM. I validated the history of this code and talked with JoeG/DougG/others to see if there was any reason beyond not having hash_combine for us not to use hash_combine.

The reason why I am changing this now is that I want to convert SILDeclRef to have another additional bit and use an OptionSet. When I noticed this... my eyes burned, so I thought I would just quickly fix it before I landed the other change so it could be a NFC change.

…use llvm::hash_combine instead of rolling its own hash combiner.

This is really old code from before llvm::hash_combine existed. We really
shouldn't be rolling out own hash combine when we have something that we are
consistently using from LLVM. I validated the history of this code and talked
with JoeG/DougG/others to see if there was any reason beyond not having
hash_combine for us not to use hash_combine.

The reason why I am changing this now is that I want to convert SILDeclRef to
have another additional bit and use an OptionSet. When I noticed this... my eyes
burned, so I thought I would just quickly fix it before I landed the other
change so it could be a NFC change.
@gottesmm gottesmm requested a review from jckarter as a code owner March 6, 2025 21:29
@gottesmm
Copy link
Contributor Author

gottesmm commented Mar 6, 2025

@swift-ci smoke test

@gottesmm
Copy link
Contributor Author

gottesmm commented Mar 7, 2025

@swift-ci smoke test linux platform

@gottesmm gottesmm enabled auto-merge March 7, 2025 20:23
@gottesmm gottesmm merged commit e06db4a into swiftlang:main Mar 7, 2025
3 checks passed
@gottesmm gottesmm deleted the pr-07b2e4050930bb96f12c1c63c958545d4bc20cea branch March 8, 2025 01:03
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.

1 participant