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

Upgrade to v4.0.0-rc2 (take two) #139

Merged
merged 22 commits into from
Jul 13, 2022
Merged

Conversation

nrabinowitz
Copy link
Collaborator

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:

  • Updates to error handling, including adding the core library code to the error and using a similar code-based approach for binding-level errors
  • Renaming various variables and internal functions to better align with v4
  • Using a C helper to support reading int64 values as JS 64-bit floats (confirmed this appears precise up to MAX_SAFE_INTEGER)
  • Test updates, particularly around testing the new errors

@coveralls
Copy link

coveralls commented Jul 6, 2022

Pull Request Test Coverage Report for Build 2631277174

  • 318 of 318 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 2543820363: 0.0%
Covered Lines: 510
Relevant Lines: 510

💛 - Coveralls

@nrabinowitz
Copy link
Collaborator Author

Note that the renaming of distance here should probably wait for uber/h3#622 to be resolved

@nrabinowitz
Copy link
Collaborator Author

Note that the renaming of distance here should probably wait for uber/h3#622 to be resolved

Actually, I walked this back in the JS code - we can update with the next release candidate.

build/sizes.c Outdated Show resolved Hide resolved
build/sizes.h Show resolved Hide resolved
lib/errors.js Outdated Show resolved Hide resolved
lib/h3core.js Outdated
* @param {number} cAddress Pointer to allocated C memory
* @return {number} Double value
*/
function readInt64FromPointer(cAddress) {
Copy link
Collaborator

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.

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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.

@nrabinowitz nrabinowitz merged commit 4ef0914 into uber:master Jul 13, 2022
@nrabinowitz nrabinowitz deleted the dev-v4 branch July 13, 2022 21:13
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

3 participants