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

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 3 commits
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 36 additions & 5 deletions h3napi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,15 +70,46 @@
if (napiArrayLen ## A ## N != napi_ok)

#define napiGetH3IndexArg(I, O) \
H3Index O;\
/*For string form input*/\
char O ## Str[17];\
size_t O ## StrCount;\
/*For typedarray form input*/\
napi_typedarray_type O ## Type;\
size_t O ## Length;\
void* O ## Data;\
/*For array form input*/\
napiGetNapiArg(I, O ## Arr);\
uint32_t O ## ArrSz;\
\
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);\
if (napi_get_value_string_utf8(env, argv[I], O ## Str, 17, &O ## StrCount) == napi_ok) {\
O = stringToH3(O ## Str);\
} else if (napi_get_typedarray_info(env, O ## Arr, &O ## Type, &O ## Length, &O ## Data, NULL, NULL) == napi_ok) {\
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 think NULL is fine here: nodejs/node#40371

if (O ## Type != napi_uint32_array || O ## Length != 2) {\
napi_throw_error(env, "EINVAL", "Invalid Uint32Array H3 index in arg " #I);\
return NULL;\
}\
O = ((H3Index)*(((uint32_t*)O ## Data) + 1) << 32) | *((uint32_t*)O ## Data);\
} else if (napi_get_array_length(env, O ## Arr, &O ## ArrSz) == napi_ok) {\
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);\
} else {\
napi_throw_error(env, "EINVAL", "Expected string or array H3 index in arg " #I);\
return NULL;\
}\
\
H3Index O = stringToH3(O ## Str);
}

#define napiGetH3Index(I, O) \
char O ## Str[17];\
Expand Down
34 changes: 34 additions & 0 deletions test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,40 @@ 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([1, 'a']), 'Array with invalid elements is not valid');
test.throws(() =>
h3node.h3IsValid([0x3fffffff, 0x8528347, 0]),
'Array with an additional element is not valid'
);
test.done();
};

exports['h3IsValid_uint32array'] = test => {
test.ok(h3node.h3IsValid(new Uint32Array([0x3fffffff, 0x8528347])), 'Integer H3 index is considered an index');
test.ok(
!h3node.h3IsValid(new Uint32Array([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(new Uint32Array([])), 'Empty array is not valid');
test.throws(() => h3node.h3IsValid(new Uint32Array([1])), 'Array with a single element is not valid');
test.throws(
() => h3node.h3IsValid(new Uint32Array([0x3fffffff, 0x8528347, 1])),
'Array with too many elements 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