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 5 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
83 changes: 77 additions & 6 deletions h3napi.c
View file
Open in desktop
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 Expand Up @@ -270,7 +301,47 @@ napiFn(h3GetBaseCell) {

napiFn(h3IsValid) {
napiArgs(1);
napiGetH3IndexArg(0, h3);
// inlined napiGetH3IndexArg in order to return false instead of throwing
H3Index h3;
/*For string form input*/
char h3Str[17];
size_t h3StrCount;
/*For typedarray form input*/
napi_typedarray_type h3Type;
size_t h3Length;
void* h3Data;
/*For array form input*/
napiGetNapiArg(0, h3Arr);
uint32_t h3ArrSz;

if (napi_get_value_string_utf8(env, argv[0], h3Str, 17, &h3StrCount) == napi_ok) {
h3 = stringToH3(h3Str);
} else if (napi_get_typedarray_info(env, h3Arr, &h3Type, &h3Length, &h3Data, NULL, NULL) == napi_ok) {
if (h3Type != napi_uint32_array || h3Length != 2) {
napiNapiBool(false, result);
return result;
}
h3 = ((H3Index)*(((uint32_t*)h3Data) + 1) << 32) | *((uint32_t*)h3Data);
} else if (napi_get_array_length(env, h3Arr, &h3ArrSz) == napi_ok) {
if (h3ArrSz != 2) {
napiNapiBool(false, result);
return result;
}
napi_value h3Val0, h3Val1;
if (napi_get_element(env, h3Arr, 0, &h3Val0) != napi_ok || napi_get_element(env, h3Arr, 1, &h3Val1) != napi_ok) {
napiNapiBool(false, result);
return result;
}
uint32_t h3Part0, h3Part1;
if (napi_get_value_uint32(env, h3Val0, &h3Part0) != napi_ok || napi_get_value_uint32(env, h3Val1, &h3Part1) != napi_ok) {
napiNapiBool(false, result);
return result;
}
h3 = ((H3Index)(h3Part1) << 32) | (h3Part0);
} else {
napiNapiBool(false, result);
return result;
}

int isValid = h3IsValid(h3);

Expand Down
58 changes: 58 additions & 0 deletions test/index.js
View file
Open in desktop
Expand Up @@ -113,6 +113,64 @@ const exportTest = (methodName, genArgs, testFn, extraName = '') =>
const exportBenchmark = (methodName, genArgs, useTryCatch = false, extraName = '') =>
exports[`${methodName}${extraName}Benchmark`] = benchmarkGen(methodName, genArgs, useTryCatch, extraName)

// h3IsValid has unique input parsing logic to return false rather than throw on invalid input
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'
);
test.ok(!h3node.h3IsValid([]), 'Empty array is not valid');
test.ok(!h3node.h3IsValid([1]), 'Array with a single element is not valid');
test.ok(!h3node.h3IsValid([1, 'a']), 'Array with invalid elements is not valid');
test.ok(!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'
);
test.ok(!h3node.h3IsValid(new Uint32Array([])), 'Empty array is not valid');
test.ok(!h3node.h3IsValid(new Uint32Array([1])), 'Array with a single element is not valid');
test.ok(!h3node.h3IsValid(new Uint32Array([0x3fffffff, 0x8528347, 1])),
'Array with too many elements is not valid'
);
test.done();
};

// Exercise the macro used to reading H3 index input from Node
exports['h3ToGeo_array'] = test => {
test.deepEqual(h3node.h3ToGeo([0x3fffffff, 0x8528347]), h3node.h3ToGeo('85283473fffffff'), 'Integer H3 index is considered an index');
test.deepEqual(h3node.h3ToGeo([0x73fffffff, 0xff2834]), h3node.h3ToGeo('ff28343fffffff'),
'Integer with incorrect bits handled consistently'
);
test.throws(() => h3node.h3ToGeo([]), 'Empty array is not valid');
test.throws(() => h3node.h3ToGeo([1]), 'Array with a single element is not valid');
test.throws(() => h3node.h3ToGeo([1, 'a']), 'Array with invalid elements is not valid');
test.throws(() => h3node.h3ToGeo([0x3fffffff, 0x8528347, 0]),
'Array with an additional element is not valid'
);
test.done();
};

exports['h3ToGeo_uint32array'] = test => {
test.deepEqual(h3node.h3ToGeo(new Uint32Array([0x3fffffff, 0x8528347])), h3node.h3ToGeo('85283473fffffff'), 'Integer H3 index is considered an index');
test.deepEqual(h3node.h3ToGeo(new Uint32Array([0x73fffffff, 0xff2834])), h3node.h3ToGeo('ff28343fffffff'),
'Integer with incorrect bits handled consistently'
);
test.throws(() => h3node.h3ToGeo(new Uint32Array([])), 'Empty array is not valid');
test.throws(() => h3node.h3ToGeo(new Uint32Array([1])), 'Array with a single element is not valid');
test.throws(() => h3node.h3ToGeo(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