Move Vec3d to its own files #67

isaacbrodsky · 2018-06-01T20:51:03Z

Follow up to @wewei's PR #63. Vec3d is moved to a new file, and added a few simple tests of those functions directly.

I added a generateFaceCenterPoint program since I like being able to regenerate tables in the library easily. :)

coveralls · 2018-06-01T20:54:06Z

Coverage remained the same at 100.0% when pulling 5e99e40 on isaacbrodsky:move-vec3d into f24c8a6 on uber:master.

nrabinowitz

Looks good - most of my questions here are just for my edification :).

nrabinowitz · 2018-06-01T23:58:09Z

src/apps/miscapps/generateFaceCenterPoint.c

+        GeoCoord centerCoords = faceCenterGeo[i];
+        Vec3d centerPoint;
+        _geoToVec3d(&centerCoords, &centerPoint);
+        printf("{%.16f, %.16f, %.16f}, // face %2d\n", centerPoint.x,


Nit: Do you need leading spaces?

I just left them for clang-format to insert. I see in the base cell neighbors generator it does have the spaces, I'll add them here for consistency.

nrabinowitz · 2018-06-01T23:59:07Z

src/apps/miscapps/generateFaceCenterPoint.c

+
+int main(int argc, char* argv[]) {
+    // check command line args
+    if (argc > 1) {


Any need for this block, when the script takes no args?

It just checks that no (unused) args were provided.

Same comment as other review (#69), you can always do int main(void) in this case. No need to be "strict" about it.

nrabinowitz · 2018-06-01T23:59:31Z

src/apps/testapps/testVec3d.c

+
+BEGIN_TESTS(Vec3d);
+
+TEST(_pointSquareDist) {


Thanks for adding tests!

nrabinowitz · 2018-06-02T00:02:43Z

src/apps/testapps/testVec3d.c

+}
+
+TEST(_geoToVec3d) {
+    Vec3d origin = {0};


For my info - why not {0, 0, 0}? Doesn't this result in undefined memory for the uninitialized struct fields? Or is this functionally equivalent to {0, 0, 0}?

I understand this should be the same as initializing all members to 0, https://stackoverflow.com/questions/11152160/initializing-a-struct-to-0

nrabinowitz · 2018-06-02T00:17:46Z

src/h3lib/lib/faceijk.c


 /** square root of 7 */
 #define M_SQRT7 2.6457513110645905905016157536392604257102L

 /** @brief icosahedron face centers in lat/lon radians */
-static const GeoCoord faceCenterGeo[NUM_ICOSA_FACES] = {
+const GeoCoord faceCenterGeo[NUM_ICOSA_FACES] = {


For my info: Why drop static?

Static is incompatible with extern, because a static variable would only be visible within the same compilation unit (think .c file) and extern is needed for the generator.

You don't need extern here if this is declared extern in a header file, which is the case here. I'd remove the extern here.

My mistake. Looks good.

nrabinowitz · 2018-06-04T15:43:16Z

Thanks for the explanations, @isaacbrodsky!

isaachier

Minor comment.

isaachier · 2018-06-04T19:39:58Z

src/h3lib/lib/faceijk.c


 /** square root of 7 */
 #define M_SQRT7 2.6457513110645905905016157536392604257102L

 /** @brief icosahedron face centers in lat/lon radians */
-static const GeoCoord faceCenterGeo[NUM_ICOSA_FACES] = {
+const GeoCoord faceCenterGeo[NUM_ICOSA_FACES] = {


You don't need extern here if this is declared extern in a header file, which is the case here. I'd remove the extern here.

isaachier

LGTM

isaachier · 2018-06-04T20:47:26Z

src/h3lib/lib/faceijk.c


 /** square root of 7 */
 #define M_SQRT7 2.6457513110645905905016157536392604257102L

 /** @brief icosahedron face centers in lat/lon radians */
-static const GeoCoord faceCenterGeo[NUM_ICOSA_FACES] = {
+const GeoCoord faceCenterGeo[NUM_ICOSA_FACES] = {


My mistake. Looks good.

Move Vec3d to its own files

nrabinowitz approved these changes Jun 2, 2018

View reviewed changes

Isaac Brodsky added 3 commits June 3, 2018 17:49

Move Vec3d to its own file and add a few tests for it specifically.

dd68a69

Add generateFaceCenterPoint

3a0b211

Add spaces before generated values

20ca294

isaacbrodsky fo 8000 rce-pushed the move-vec3d branch from 5cca316 to 20ca294 Compare June 4, 2018 00:49

isaachier reviewed Jun 4, 2018

View reviewed changes

isaachier approved these changes Jun 4, 2018

View reviewed changes

Add CHANGELOG for moving Vec3d

5e99e40

isaacbrodsky merged commit abbc693 into uber:master Jun 5, 2018

isaacbrodsky deleted the move-vec3d branch June 5, 2018 00:24

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

Merge pull request uber#67 from isaacbrodsky/move-vec3d

27bc065

Move Vec3d to its own files

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Vec3d to its own files #67

Move Vec3d to its own files #67


		BEGIN_TESTS(Vec3d);

		TEST(_pointSquareDist) {

Move Vec3d to its own files #67

Move Vec3d to its own files #67

Conversation

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

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