WARNING: THIS SITE IS A MIRROR OF GITHUB.COM / IT CANNOT LOGIN OR REGISTER ACCOUNTS / THE CONTENTS ARE PROVIDED AS-IS / THIS SITE ASSUMES NO RESPONSIBILITY FOR ANY DISPLAYED CONTENT OR LINKS / IF YOU FOUND SOMETHING MAY NOT GOOD FOR EVERYONE, CONTACT ADMIN AT ilovescratch@foxmail.com
Skip to content

Conversation

@KitaroMoha563
Copy link
Contributor

PointerType hashed addrspace directly but compared via address_space(), causing equal values to have different hashes.

Copy link
Collaborator

@bitwalker bitwalker left a comment

Choose a reason for hiding this comment

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

The test coverage here is unnecessary IMO, or at least, doesn't feel like the value of them balances out the amount of noise they introduce. If we actually wanted to validate that we aren't making mistakes like this with Rust's various traits, we'd need an enormous number of such tests, and that's just not a valuable use of time as far as I'm concerned.

Otherwise, looks good - if you can remove the test code from this PR, leaving just the patch of the Hash impl, I'll merge.

@KitaroMoha563
Copy link
Contributor Author

Hi @bitwalke, removed the test code, pls check ,thanks!

@plafer plafer added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label Dec 10, 2025
@plafer
Copy link
Contributor

plafer commented Dec 10, 2025

Hey @KitaroMoha563 can you sign your commits? Then we can merge.

@KitaroMoha563 KitaroMoha563 force-pushed the fix-pointer-hash branch 2 times, most recently from 8d00eb1 to d216663 Compare December 11, 2025 01:29
@KitaroMoha563
Copy link
Contributor Author

@plafer did it, sir.

@plafer plafer merged commit 59d1e82 into 0xMiden:next Dec 11, 2025
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no changelog This PR does not require an entry in the `CHANGELOG.md` file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants