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.

Sign up for GitHub

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

Jump to bottom

Integer array API #40

Merged
merged 6 commits into from Oct 23, 2021
Merged

Integer array API #40

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7bd4427
Integer array input, as in uber/h3-js#91
Oct 5, 2021
2ec5868
Change per review
Oct 6, 2021
c793773
Support Uint32Array
Oct 8, 2021
bf71ea8
h3IsValid type should not throw
Oct 21, 2021
b001f43
Add tests for H3Index parsing outside of h3IsValid
Oct 23, 2021
cd1c99d
Change assertion per review
Oct 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 26 additions & 6 deletions h3napi.c
View file
Open in desktop
Expand Up @@ -70,15 +70,35 @@
if (napiArrayLen ## A ## N != napi_ok)

#define napiGetH3IndexArg(I, O) \
H3Index O;\
char O ## Str[17];\
size_t O ## StrCount;\
\
if (napi_get_value_string_utf8(env, argv[I], O ## Str, 17, &O ## StrCount) != napi_ok) {\
napi_throw_error(env, "EINVAL", "Expected string h3 index in arg " #I);\
return NULL;\
}\
\
H3Index O = stringToH3(O ## Str);
if (napi_get_value_string_utf8(env, argv[I], O ## Str, 17, &O ## StrCount) == napi_ok) {\
O = stringToH3(O ## Str);\
} else {\
Copy link
Owner

Choose a reason for hiding this comment

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

can you make this an else if checking for whether or not the input is an array and then restore the error message above for the else path (now saying must be a string or pair of 32-bit integers in an array)?

That would also make it amenable to eventually supporting BigInt inputs in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I refactored to make that flow clearer.

napiGetNapiArg(I, O ## Arr);\
uint32_t O ## ArrSz;\
if (napi_get_array_length(env, O ## Arr, &(O ## ArrSz)) != napi_ok) {\
napi_throw_error(env, "EINVAL", "Expected string or array H3 index in arg " #I);\
return NULL;\
}\
if (O ## ArrSz != 2) {\
napi_throw_error(env, "EINVAL", "Invalid length array H3 index in arg " #I);\
return NULL;\
}\
napi_value O ## Val0, O ## Val1;\
if (napi_get_element(env, O ## Arr, 0, &(O ## Val0)) != napi_ok || napi_get_element(env, O ## Arr, 1, &(O ## Val1)) != napi_ok) {\
napi_throw_error(env, "EINVAL", "Invalid array H3 index in arg " #I);\
return NULL;\
}\
uint32_t O ## Part0, O ## Part1;\
if (napi_get_value_uint32(env, O ## Val0, &(O ## Part0)) != napi_ok || napi_get_value_uint32(env, O ## Val1, &(O ## Part1)) != napi_ok) {\
napi_throw_error(env, "EINVAL", "Invalid integer array H3 index in arg " #I);\
return NULL;\
}\
O = ((H3Index)(O ## Part1) << 32) | (O ## Part0);\
}

#define napiGetH3Index(I, O) \
char O ## Str[17];\
Expand Down
16 changes: 16 additions & 0 deletions test/index.js
View file
Open in desktop
Expand Up @@ -113,6 +113,22 @@ const exportTest = (methodName, genArgs, testFn, extraName = '') =>
const exportBenchmark = (methodName, genArgs, useTryCatch = false, extraName = '') =>
exports[`${methodName}${extraName}Benchmark`] = benchmarkGen(methodName, genArgs, useTryCatch, extraName)

exports['h3IsValid_array'] = test => {
test.ok(h3node.h3IsValid([0x3fffffff, 0x8528347]), 'Integer H3 index is considered an index');
Copy link

Choose a reason for hiding this comment

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

Nit: For methods expected to return boolean, prefer t.equal(result, false) etc to avoid tests passing on truthy/falsy values like undefined etc.

test.ok(
!h3node.h3IsValid([0x73fffffff, 0xff2834]),
'Integer with incorrect bits is not considered an index'
);
// TODO: This differs from h3-js, in these cases h3-js would return false rather than throwing
test.throws(() => h3node.h3IsValid([]), 'Empty array is not valid');
Copy link

Choose a reason for hiding this comment

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

This seems like incorrect behavior. Is there a way to wrap in a try/catch? Are we getting these errors from your new code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, although these cases will all throw in h3-node as it is. We would need to change the behavior of h3-node to match h3-js here. Further, these cases will throw in other functions, too, unlike in h3-js where [0, 0] is used.

Copy link

Choose a reason for hiding this comment

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

I guess this is up to @dfellis, as it's an API question, but my assumption is that h3IsValid should never throw for invalid input, since it's the job of the function to recognize it. We could check for valid C input in JS and return false before sending to C and getting an error.

Copy link
Owner

Choose a reason for hiding this comment

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

So these bindings directly expose the functions without a Javascript wrapper and that would go against the perf angle for this repo.

This particular C function seems like it would be relatively trivial to replace throwing an error with returning false, while some of the others would be more difficult.

Is the objection primarily for h3IsValid or for any deviation wrt h3-js? The former I think @isaacbrodsky or myself could quickly fix in this PR, the latter I would prefer to happen in a different PR.

Copy link

Choose a reason for hiding this comment

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

I was just thinking of h3IsValid, because presumably folks might use it to check untrusted data before passing it to another H3 function. In general, I'd expect a validator to either return false or throw, but not both for different cases, and particularly with the is- prefix it seems like it should never throw on bad data. As a developer, I'd much prefer something like indexes.filter(h3IsValid) than

indexes.filter(idx => {
  try {
    return h3IsValid(idx);
  } catch (e) {
    return false;
  }
})

test.throws(() => h3node.h3IsValid([1]), 'Array with a single element is not valid');
test.throws(() =>
h3node.h3IsValid([0x3fffffff, 0x8528347, 0]),
'Array with an additional element is not valid'
);
test.done();
};

exportTest('geoToH3', () => [...randCoords(), 9], simpleTest)
exportTest('h3ToGeo', () => [h3node.geoToH3(...randCoords(), 9)], almostEqualTest)
exportTest('h3ToGeoBoundary', () => [h3node.geoToH3(...randCoords(), 9)], almostEqualTest)
Expand Down