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

Fixed #66: equal numbers issue different hashes #67

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

spektom
Copy link

@spektom spektom commented Mar 7, 2021

The idea is to use decQuadReduce() function that eliminates insignificant zeroes from the number before hashing.

@jonathanstrong
Copy link
Collaborator

hey - just looking at this. thank you for submitting the pull request.

I did have one question, I'm not familiar with what trim/decNumberTrim does. I realize you switched to reduce, which makes sense, but is there an independent function of trim that we possibly want to offer for its own reasons?

@spektom
Copy link
Author

spektom commented Mar 9, 2021

@jonathanstrong hey, sorry I don't follow. Are you asking whether trim should be part of Rust API?

@jonathanstrong
Copy link
Collaborator

potentially? the C code has a comment describing decNumberTrim as "remove insignificant zeros", while the comment for decNumberReduce is "remove trailing zeros". I don't understand how these are different in terms of functionality, and would like to for the purposes of assessing whether the Rust api should also provide a trim method on d128.

@spektom
Copy link
Author

spektom commented Mar 9, 2021

Frankly, I don't know. decNumberTrim is a utility function as it's called internally, I'm not sure it should be part of the API. Anyway, I don't think this decision should be made as part of this PR.

@spektom
Copy link
Author

spektom commented Mar 21, 2021

@jonathanstrong is there a concern regarding usage of decNumberReduce()?

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.

None yet

2 participants