-
Notifications
You must be signed in to change notification settings - Fork 80
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
Upgrade to v4.0.0-rc2 (take two) #139
Conversation
Pull Request Test Coverage Report for Build 2631277174
💛 - Coveralls |
Note that the renaming of |
Actually, I walked this back in the JS code - we can update with the next release candidate. |
lib/h3core.js
Outdated
* @param {number} cAddress Pointer to allocated C memory | ||
* @return {number} Double value | ||
*/ | ||
function readInt64FromPointer(cAddress) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the function name should indicate a cast is happening. That kind of precision losing cast can be an unexpected operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I can update to readInt64AsDoubleFromPointer
- a little wordy, but accurate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's worth noting though that losing precision will be very rare - the total count of res 15 tiles is still lower than MAX_SAFE_INTEGER
. I can't think of a scenario in our current usage where we would deal with an int64
value that's bigger than this.
Supersedes #136
@isaacbrodsky and I agreed that I should take on the subsequent work on his PR, and with separate forks the easiest option was to open a new PR with his commits + my commits. For the most part, my commits cover my own code review comments on the previous PR.
Summary of my additional changes:
int64
values as JS 64-bit floats (confirmed this appears precise up toMAX_SAFE_INTEGER
)