recp / cglm

📽 Highly Optimized 2D / 3D Graphics Math (glm) for C
MIT License
2.35k stars 231 forks source link

Adding `.clang-format` ?? #433

Open MarcinKonowalczyk opened 1 day ago

MarcinKonowalczyk commented 1 day ago

The project formatting seems a little bit inconsistent between files. I wonder is it worth adding .clang-format to attempt to unify the formatting? After playing around with one for a bit I've arrived at:

---
BasedOnStyle: GNU
IndentWidth: 2
SortIncludes: false
AlignAfterOpenBracket: Align
AllowAllParametersOfDeclarationOnNextLine: false
BreakBeforeBraces: Attach
PointerAlignment: Middle
ColumnLimit: 0
IndentPPDirectives: AfterHash
Cpp11BracedListStyle: true
SpaceBeforeParens: Custom

AlignConsecutiveMacros: Consecutive
AlignOperands: AlignAfterOperator

SpaceBeforeParensOptions:
  AfterControlStatements: false
  AfterFunctionDeclarationName: false
  AfterFunctionDefinitionName: false
  BeforeNonEmptyParentheses: false

which, seems to cause minimal stress to the codebase. After running clang-format -style=file ./src/**/*.c ./include/**/*.h -i (clang-format version is 19.1.4 from homebrew), i get a pretty minor diff in 140 files. The c files are basically untouched, except for a couple of spaces for param alignment, for example:

diff --git a/src/euler.c b/src/euler.c
index 8749ba5..b093b3d 100644
--- a/src/euler.c
+++ b/src/euler.c
@@ -25 +25 @@ void
-glmc_euler_xyz(vec3 angles,  mat4 dest) {
+glmc_euler_xyz(vec3 angles, mat4 dest) {

The h files are also mostly ok, but a couple of places suffer a tiny bit from clang-format limitations, for example:

@@ -103,3 +103,3 @@ glm_ortho_aabb_p_rh_zo(vec3 box[2], float padding, mat4 dest) {
-  glm_ortho_rh_zo(box[0][0] - padding,    box[1][0] + padding,
-                  box[0][1] - padding,    box[1][1] + padding,
-                -(box[1][2] + padding), -(box[0][2] - padding),
+  glm_ortho_rh_zo(box[0][0] - padding, box[1][0] + padding,
+                  box[0][1] - padding, box[1][1] + padding,
+                  -(box[1][2] + padding), -(box[0][2] - padding),

where it does not understand the alignment of the box.

Also, i've skipped formatting of the tests, for now, since clang-format is a tiny bit confused about the ASSERT macro and, in some cases it messes up the indentation, for example:

diff --git a/test/src/test_mat2.h b/test/src/test_mat2.h
index 6fa622a..a872b56 100644
--- a/test/src/test_mat2.h
+++ b/test/src/test_mat2.h
@@ -10,2 +10,2 @@
-#define A_MATRIX2x2 {{1,2},{5,6}}
-#define MAT2_ARRAY {1, 5, 2, 7}
+#define A_MATRIX2x2 {{1, 2}, {5, 6}}
+#define MAT2_ARRAY  {1, 5, 2, 7}
@@ -14 +14 @@
-#define CGLM_TEST_MAT2_ONCE
+#  define CGLM_TEST_MAT2_ONCE
@@ -18 +18 @@ TEST_IMPL(MACRO_GLM_MAT2_IDENTITY_INIT) {
-  
+
@@ -27 +27 @@ TEST_IMPL(MACRO_GLM_MAT2_IDENTITY_INIT) {
-TEST_IMPL(MACRO_GLM_MAT2_ZERO_INIT) {  
+TEST_IMPL(MACRO_GLM_MAT2_ZERO_INIT) {
@@ -29 +29 @@ TEST_IMPL(MACRO_GLM_MAT2_ZERO_INIT) {
-  
+
@@ -38,5 +38,5 @@ TEST_IMPL(MACRO_GLM_MAT2_ZERO_INIT) {
-TEST_IMPL(MACRO_GLM_MAT2_IDENTITY) {
-  ASSERT(test_eq(GLM_MAT2_IDENTITY[0][0], 1.0f))
-  ASSERT(test_eq(GLM_MAT2_IDENTITY[0][1], 0.0f))
-  ASSERT(test_eq(GLM_MAT2_IDENTITY[1][0], 0.0f))
-  ASSERT(test_eq(GLM_MAT2_IDENTITY[1][1], 1.0f))
+TEST_IMPL(MACRO_GLM_MAT2_IDENTITY){
+    ASSERT(test_eq(GLM_MAT2_IDENTITY[0][0], 1.0f))
+        ASSERT(test_eq(GLM_MAT2_IDENTITY[0][1], 0.0f))
+            ASSERT(test_eq(GLM_MAT2_IDENTITY[1][0], 0.0f))
+                ASSERT(test_eq(GLM_MAT2_IDENTITY[1][1], 1.0f))

These work ok though, after adding ; after ASSERT (which is protected by do {...} while (0), so it does not change the functionality.


Overall, if we were to add .clang-format, I'd suggest doing it in two commits, 1) adding .clang-format and making any small changes to the codebase in preparation for 2) actually running the formatter. This way the commit with the formatter can be easily skipped in any potential git blame searches or anything of this sort. By the small changes in step 1 i mean things like the ; after assets and any /* clang-format on/off */ blocks we might want to add to preserve the indentation in places like glm_ortho_aabb_p_rh_zo above.

MarcinKonowalczyk commented 1 day ago

Oh, and just to check I've run the tests (mkdir build && cd build && cmake -DCGLM_USE_TEST=ON .. && make && ./tests) and they all pass no problem.

MarcinKonowalczyk commented 1 day ago

As a background, I've come across this when fighting with my auto-format option in vscode while working on another pr/proposal for this project. I could not get it to have a consistent style based on other files I've looked at, and the config above is the one i've eventually landed on.

MarcinKonowalczyk commented 1 day ago

Update: Adding StatementMacros: ["TEST_ENTRY", "ASSERT"] fixes formatting for these two macros. No ;'s needed.

MarcinKonowalczyk commented 1 day ago

And here is the output of git diff --ignore-all-space --ignore-blank-lines --compact-summary:

 include/cglm/affine-mat.h              |   3 +-
 include/cglm/affine-post.h             |   3 +-
 include/cglm/affine-pre.h              |   6 +-
 include/cglm/affine.h                  |  27 +++++--
 include/cglm/clipspace/project_no.h    |   3 +-
 include/cglm/clipspace/project_zo.h    |   3 +-
 include/cglm/clipspace/view_lh.h       |   9 ++-
 include/cglm/clipspace/view_rh.h       |   9 ++-
 include/cglm/ease.h                    |   3 +-
 include/cglm/euler.h                   |  83 ++++++++++++++-------
 include/cglm/frustum.h                 |   3 +-
 include/cglm/handed/euler_to_quat_lh.h |  54 +++++++++-----
 include/cglm/handed/euler_to_quat_rh.h |  54 +++++++++-----
 include/cglm/io.h                      |  45 +++++++++---
 include/cglm/mat2x3.h                  |  17 +++--
 include/cglm/mat2x4.h                  |  22 ++++--
 include/cglm/mat3.h                    |  12 ++-
 include/cglm/mat3x2.h                  |  16 +++-
 include/cglm/mat3x4.h                  |  31 ++++++--
 include/cglm/mat4.h                    | 129 +++++++++++++++++++++++----------
 include/cglm/mat4x2.h                  |  10 ++-
 include/cglm/mat4x3.h                  |  18 +++--
 include/cglm/quat.h                    |  93 +++++++++++++++++-------
 include/cglm/simd/arm.h                |  54 ++++++--------
 include/cglm/simd/neon/quat.h          |   3 +-
 include/cglm/simd/wasm.h               |  74 +++++++------------
 include/cglm/simd/x86.h                |  89 +++++++++--------------
 include/cglm/struct/quat.h             |   3 +-
 include/cglm/types-struct.h            |   6 +-
 include/cglm/vec2.h                    |   3 +-
 include/cglm/vec4.h                    |   6 +-
 test/include/common.h                  |   8 +-
 test/runner.c                          |   4 +-
 test/src/test_affine_mat.h             |   3 +-
 test/src/test_mat2.h                   |   6 +-
 test/src/test_mat2x3.h                 |   3 +-
 test/src/test_mat2x4.h                 |   3 +-
 test/src/test_mat3.h                   |  15 ++--
 test/src/test_mat3x2.h                 |   3 +-
 test/src/test_mat3x4.h                 |   3 +-
 test/src/test_mat4.h                   |  26 +++++--
 test/src/test_mat4x2.h                 |   3 +-
 test/src/test_mat4x3.h                 |   3 +-
 test/src/test_quat.h                   |   3 +-
 test/src/test_vec2.h                   |  18 ++---
 test/src/test_vec3.h                   |   9 ++-
 test/src/test_vec4.h                   |  24 ++----
 47 files changed, 618 insertions(+), 407 deletions(-)

The changes are mainly due to clang-format not understanding things like:

  m[0][0] += c;       m[1][0] -= vs[2];   m[2][0] += vs[1];
  m[0][1] += vs[2];   m[1][1] += c;       m[2][1] -= vs[0];
  m[0][2] -= vs[1];   m[1][2] += vs[0];   m[2][2] += c;

Very fixable with /* clang-format on/off */