Skip to content

Add error return codes to compactCells#468

Merged
isaacbrodsky merged 7 commits intouber:masterfrom
isaacbrodsky:error-returns-compact
Jun 3, 2021
Merged

Add error return codes to compactCells#468
isaacbrodsky merged 7 commits intouber:masterfrom
isaacbrodsky:error-returns-compact

Conversation

@isaacbrodsky
Copy link
Copy Markdown
Collaborator

No description provided.

@coveralls
Copy link
Copy Markdown

coveralls commented May 20, 2021

Coverage Status

Coverage increased (+0.0006%) to 98.992% when pulling a23cea4 on isaacbrodsky:error-returns-compact into 357d1b7 on uber:master.

@isaacbrodsky isaacbrodsky force-pushed the error-returns-compact branch from 227e7d6 to 879ac99 Compare May 21, 2021 17:36
int64_t childrenSz = H3_EXPORT(uncompactCellsSize)(&origin, 1, 2);
int64_t childrenSz;
t_assertSuccess(
H3_EXPORT(uncompactCellsSize)(&origin, 1, 2, &childrenSz));
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

For my info: Is there a clear guideline on when to use t_assertSuccess and when to use t_assert(foo(bar) == E_SUCCESS)? I'm assuming that the former is for expected successes in setup code, while the latter is for expected successes in the logic actually under test.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is rouhgly my usage. There may be some cases where I use t_assertSuccess for logic under test, but usually not so as to preserve the message. I think it can be treated as just a convenient way to write the t_assert invocation, as the messages are fairly opaque anyways (unless the message is used to clarify what is going on in the test)

Comment thread src/h3lib/lib/h3Index.c Outdated

for (int64_t j = 0; j < numCompacted; j++) {
if (!_hasChildAtRes(compactedSet[j], res)) return -2;
if (!_hasChildAtRes(compactedSet[j], res)) return E_FAILED;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: Do we want a more specific error here? E.g. E_RES_INVALID?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I used the E_RES_MISMATCH for these cases, since it covers the H3Index res being invalid. There is a bit of overlap between E_RES_MISMATCH (the index resolution is out of range) and E_RES_DOMAIN (the resolution argument itself is out of range), which is tricky.

Comment thread src/h3lib/lib/h3Index.c Outdated
for (int64_t i = 0; i < numCompacted; i++) {
if (compactedSet[i] == H3_NULL) continue;
if (!_hasChildAtRes(compactedSet[i], res)) return -1; // Abort
if (!_hasChildAtRes(compactedSet[i], res)) return E_FAILED; // Abort
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ditto

Uncompacts the set `compactedSet` of indexes to the resolution `res`. `h3Set` must be at least of size `uncompactCellsSize(compactedSet, numHexes, res)`.

Returns 0 on success.
Returns 0 (`E_SUCCESS`) on success.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

... or non-zero error code on failure? Ideally we'd list out known errors here, linking to the error docs.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Still thinking about how to add the known failure codes in here, but I agree it would be a useful addition.

@isaacbrodsky isaacbrodsky force-pushed the error-returns-compact branch from 2a886a5 to 49a9a00 Compare June 3, 2021 16:50
Comment thread website/docs/core-library/usage.md Outdated
## API Versioning

The public API of the H3 Core Library is defined in the file `h3api.h`. The functions defined in h3api.h adhere to [Semantic Versioning](http://semver.org/).
The public API of the H3 Core Library is defined in the file [`h3api.h`](https://github.com/uber/h3/blob/master/src/h3lib/include/h3api.h.in). The functions defined in `h3api.h` adhere to [Semantic Versioning](http://semver.org/).
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Since we're linking to the .in file that we preprocess (not using the C preprocessor) maybe we need to explain that a bit?

@isaacbrodsky isaacbrodsky merged commit 9ab1053 into uber:master Jun 3, 2021
@isaacbrodsky isaacbrodsky deleted the error-returns-compact branch June 3, 2021 18:37
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.

4 participants