Switch `int` digit to an enum, `Direction` #77

isaacbrodsky · 2018-06-11T17:08:57Z

Attempt to clarify when a direction digit is expected and when it is not. C permits arithmetic and assignment of ints/enums so this is not able to catch many issues at compile time.

The neighbor traversal tables in algos.c (like NEW_DIGIT_II) could also be updated to use the constants.

Also update copyright headers and replace 0 with H3_INVALID_INDEX when applicable.

coveralls · 2018-06-11T17:12:39Z

Coverage remained the same at 100.0% when pulling 28bd51b on isaacbrodsky:direction-enum into 1d3360f on uber:master.

isaachier

Very thorough cleanup. Much more readable now! LGTM overall, just left one comment.

isaachier · 2018-06-11T18:24:29Z

src/h3lib/lib/algos.c

@@ -222,32 +223,32 @@ H3Index h3NeighborRotations(H3Index origin, int dir, int* rotations) {
        } else {
            // generated by hand
            // Current digit -> direction -> new digit
-            const int NEW_DIGIT_II[7][7] = {
+            const Direction NEW_DIGIT_II[7][7] = {
                {0, 1, 2, 3, 4, 5, 6}, {1, 4, 3, 6, 5, 2, 0},
                {2, 3, 1, 4, 6, 0, 5}, {3, 6, 4, 5, 0, 1, 2},
                {4, 5, 6, 0, 2, 3, 1}, {5, 2, 0, 1, 3, 6, 4},
                {6, 0, 5, 2, 1, 4, 3}};


Is this generated code? It would have been nice to see this as enum members instead of raw integers, but I understand this is too much to ask for all of these values.

These tables were written by hand. I'll probably update the PR to make them constants as well, although I'm not sure that'll help readability.

jogly · 2018-06-11T18:44:25Z

src/h3lib/include/coordijk.h

+    /** H3 digit in j-axes direction */
+    J_AXES_DIGIT = 2,
+    /** H3 digit in j == k direction */
+    JK_AXES_DIGIT = K_AXES_DIGIT | J_AXES_DIGIT, /* 3 */


tiniest nit ever: swap j & k OR to J_AXES_DIGIT | K_AXES_DIGIT s.t. JK_... = J_... | K_... :P

nrabinowitz

A few nits, but overall I really like this change.

nrabinowitz · 2018-06-11T19:03:43Z

src/apps/miscapps/generateBaseCellNeighbors.c

@@ -175,15 +175,15 @@ static void generate() {
                        if (i == 4 || i == 117) {
                            // 'polar' type pentagon with all faces pointing
                            // towards i
-                            if (dir == 5) {
+                            if (dir == IK_AXES_DIGIT) {
                                rotAdj = 2;


Can these rotAdj assignments get moved to constants as well?

nrabinowitz · 2018-06-11T19:05:33Z

src/h3lib/lib/algos.c

@@ -118,7 +119,7 @@ void H3_EXPORT(kRingDistances)(H3Index origin, int k, H3Index* out,
        // Fast algo failed, fall back to slower, correct algo
        // and also wipe out array because contents untrustworthy
        for (int i = 0; i < maxIdx; i++) {
-            out[i] = 0;
+            out[i] = H3_INVALID_INDEX;
            distances[i] = 0;


Not required, but it's worth noting that 0 is actually a valid value in distances - maybe worth defining INVALID_DISTANCE = -1 or similar?

nrabinowitz · 2018-06-11T19:35:38Z

src/h3lib/lib/coordijk.c

@@ -354,8 +349,8 @@ void _downAp7r(CoordIJK* ijk) {
 * @param ijk The ijk coordinates.
 * @param digit The digit direction from the original ijk coordinates.
 */
-void _neighbor(CoordIJK* ijk, int digit) {
-    if (digit != 0) {
+void _neighbor(CoordIJK* ijk, Direction digit) {


Nit: Should this argument be dir now? It seems like we're a little inconsistent still about whether we call this a "direction" (the semantic name) or a "digit" (the concrete implementation).

Even worse, the directions are themselves called "digits"

nrabinowitz · 2018-06-11T19:39:34Z

src/h3lib/lib/coordijk.c

-    int digit = INVALID_DIGIT;
-    for (int i = 0; i < 7; i++) {
+    Direction digit = INVALID_DIGIT;
+    for (Direction i = CENTER_DIGIT; i < INVALID_DIGIT; i++) {


Nit: This makes me think INVALID_DIGIT is the wrong name. I'd expect INVALID_X to be an opaque value, but here you have to know what the value is (or at least that it's an upper bound). Maybe OUT_OF_RANGE_DIGIT? Or define MAX_DIGIT = IJ_AXES_DIGIT and use MAX_DIGIT + 1 here?

END_ or LAST_ seem like common patterns in C, I can update to use that instead of (or perhaps as an alias to) INVALID_DIGIT.

Makes sense - I think if you use LAST_DIGIT you'd need to use LAST_DIGIT + 1 to indicate the out-of-bounds value, since LAST_DIGIT sounds valid.

I've actually seen most frequently NUM_DIRECTIONS or the equivalent.

nrabinowitz · 2018-06-11T19:47:15Z

src/h3lib/lib/h3Index.c

@@ -446,12 +446,12 @@ int H3_EXPORT(h3IsPentagon)(H3Index h) {
 * @param h The H3Index.
 * @return The highest resolution non-zero digit in the H3Index.
 */
-int _h3LeadingNonZeroDigit(H3Index h) {
+Direction _h3LeadingNonZeroDigit(H3Index h) {
    for (int r = 1; r <= H3_GET_RESOLUTION(h); r++)
        if (H3_GET_INDEX_DIGIT(h, r)) return H3_GET_INDEX_DIGIT(h, r);


Nit: As above, this requires knowledge that the center digit is 0. Maybe slightly more semantic to rewrite this as _h3LeadingNonCenterDigit and compare if (H3_GET_INDEX_DIGIT(h, r) !== CENTER_DIGIT)? No strong feeling though, it might not be possible/desirable to fully abstract away the concrete implementation.

Attempt to clarify when a direction digit is expected and when it is not. C permits arithmetic and assignment of ints/enums so this is not able to catch many issues at compile time. Also update copyright headers and replace 0 with H3_INVALID_INDEX when applicable.

The aperture 7 traversal tables are now file level constants, and use the same constants as elsewhere. Also fix JK_AXES_DIGIT ordering

isaacbrodsky · 2018-06-11T22:58:07Z

Rebased on master and addressed comments

Also fix formatting and fix another instance where an untyped direction is used

Switch `int` digit to an enum, `Direction`

isaacbrodsky requested review from nrabinowitz, dfellis and jogly June 11, 2018 17:08

isaachier reviewed Jun 11, 2018

View reviewed changes

isaachier approved these changes Jun 11, 2018

View reviewed changes

jogly approved these changes Jun 11, 2018

View reviewed changes

nrabinowitz approved these changes Jun 11, 2018

View reviewed changes

Isaac Brodsky added 3 commits June 11, 2018 15:50

Add CHANGELOG for Direction enum

b3a722d

Extract NEW_DIGIT_II, etc from h3NeighborRotations

c7e734e

The aperture 7 traversal tables are now file level constants, and use the same constants as elsewhere. Also fix JK_AXES_DIGIT ordering

isaacbrodsky force-pushed the direction-enum branch from 66e372e to c7e734e Compare June 11, 2018 22:57

Isaac Brodsky added 2 commits June 11, 2018 17:07

Add LAST_DIGIT

f67dfaa

Rename to NUM_DIGITS

28bd51b

Also fix formatting and fix another instance where an untyped direction is used

isaacbrodsky merged commit b9949b5 into uber:master Jun 15, 2018

isaacbrodsky deleted the direction-enum branch June 15, 2018 16:24

mrdvt92 pushed a commit to mrdvt92/h3 that referenced this pull request Jun 19, 2022

Merge pull request uber#77 from isaacbrodsky/direction-enum

01694e9

Switch `int` digit to an enum, `Direction`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch `int` digit to an enum, `Direction` #77

Switch `int` digit to an enum, `Direction` #77

isaacbrodsky commented Jun 11, 2018

coveralls commented Jun 11, 2018 •

edited

Loading

isaachier left a comment

isaachier Jun 11, 2018

isaacbrodsky Jun 11, 2018

jogly Jun 11, 2018

nrabinowitz left a comment

nrabinowitz Jun 11, 2018

nrabinowitz Jun 11, 2018

nrabinowitz Jun 11, 2018

isaacbrodsky Jun 11, 2018

nrabinowitz Jun 11, 2018

isaacbrodsky Jun 11, 2018

nrabinowitz Jun 11, 2018

isaachier Jun 11, 2018

nrabinowitz Jun 11, 2018

isaacbrodsky commented Jun 11, 2018

Switch int digit to an enum, Direction #77

Switch int digit to an enum, Direction #77

Conversation

isaacbrodsky commented Jun 11, 2018

coveralls commented Jun 11, 2018 • edited Loading

isaachier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrabinowitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaacbrodsky commented Jun 11, 2018

Switch `int` digit to an enum, `Direction` #77

Switch `int` digit to an enum, `Direction` #77

coveralls commented Jun 11, 2018 •

edited

Loading