-
Notifications
You must be signed in to change notification settings - Fork 38.6k
CCoinsKeyHasher::operator() should return size_t #4635
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
|
Any chance of a regression test added to the test suite? |
|
@jgarzik The tests already fail without this, on the affected environments. |
|
Right, the tests already failed. But no one has been running them with that version of boost, at least on a 32-bit system. The Boost version used by the pulltester is even older than 1.46, and it passed: It's also fine in 1.54+. Seemingly the problem with non-size_t-hashers only exists in a relatively small range of boost versions. |
|
Not sure I understand, but |
|
@Diapolo GetHash() is a 64-bit hash function so it will always give a 64-bit result. |
|
@laanwj I would feel better if you could add a small comment about that and why it was changed. |
|
Will add a comment. |
It currently returns uint64_t, which on older boost (at least 1.46) causes test failures on 32-bit systems. This problem was introduced in bc42503. Fixes bitcoin#4634.
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4635_6c23b082033b627f31170166c07ab35fa6be9343/ for binaries and test log. |
|
ACK, thanks for adding that comment. |
6c23b08 CCoinsKeyHasher::operator() should return size_t (Wladimir J. van der Laan)
|
@sipa Since |
It currently returns uint64_t, which on older boost (at least 1.46) causes failures on 32-bit systems. This problem was introduced in bc42503.
Fixes #4634.