Add clang-format support #913

MathiasMagnus · 2024-05-29T15:08:37Z

This work if the continuation of #889.

Notes for the reviewers

Some comments from the original work addressed here, most notably:

Limit column size of all formatted sources to 100.

Aside from explicit comments:

A git-blame ignore file has been committed to the repo which instructs GitHub's blame functionality on the web interface to keep attribution of lines affected by formatting. Local clones may be configured told similarly.
The functional changes are all outside the singular "Apply initial formatting" commit.

Should other formatting changes need refining, discussions may continue here.

.github/workflows/windows.yml

MarkCallow · 2024-06-05T06:00:21Z

.git-blame-ignore-revs

+# SPDX-License-Identifier: Apache-2.0
+
+# Apply initial formatting
+4ceab17e94e73c918130a49ad1b21d5e4c14e7ec


Isn't this ignoring just this single commit? What about future reformatting that takes place when commit(s) are pushed?

Thanks for reminding me, this commit hash needs update after rebases I done.

Ideally no further commits need to be added here, because now the entire repo is formatted and all formatting violations will trip CI, hence no pure formatting-related commits need to be made. Contributors are highly encouraged to configure their development environment to format on save. (eg. VS Code has editor.formatOnSave option and multiple C/C++ formatting extensions available, most notably ms-vscode.cpptools or llvm-vs-code-extensions.vscode-clangd)

Please document how to configure the development environment to configure on save. I have a personal interest in how to do it with Xcode and Vim.

Actually before you do that, let's discuss which is preferable and why: format on save from an editor or format on commit via a Git clean filter?

Did you update the commit hash?

It's been updated with the latest rebase.

Sorry. I edited my original comment that said "please resolve" to add the request about documentation which you probably missed. Probably BUILDING.md is the best place for the documentation.

Documented VS Code. I looked into (Neo)vim, but the phase space is too large.

interface/basisu_c_binding/inc/basisu_c_binding.h

MathiasMagnus · 2024-06-07T15:30:00Z

I think this work is complete. The current CI failure doesn't seem to be related to the changes made here.

MarkCallow · 2024-06-19T07:12:54Z

external/.clang-format

+---
+# Disable clang-format in this directory
+DisableFormat: true
+SortIncludes: false


Why is this needed when formatting is disabled?

It's probably not strictly necessary.

This is the convention used in other Khronos repositories, but it should have no effects (at least on clang-format versions I've used).

MarkCallow · 2024-06-19T07:17:31Z

interface/basisu_c_binding/src/basisu_c_binding.cpp

+        if (transcoder_format == transcoder_texture_format::cTFPVRTC1_4_RGB ||
+            transcoder_format == transcoder_texture_format::cTFPVRTC1_4_RGBA) {
+            // For PVRTC1, Basis only writes (or requires) total_blocks * bytes_per_block. But GL
+            // requires extra padding for very small textures:
            // https://www.khronos.org/registry/OpenGL/extensions/IMG/IMG_texture_compression_pvrtc.txt
            // The transcoder will clear the extra bytes followed the used blocks to 0.


Unrelated to this PR but given the overhead of running a PR please fix the typo. s/followed/following/.

MarkCallow · 2024-06-19T07:27:35Z

lib/astc_encode.cpp

-#define WIN32_LEAN_AND_MEAN
-#include <Windows.h>
+    #define WIN32_LEAN_AND_MEAN
+    #include <Windows.h>

 typedef HANDLE pthread_t;


Can you get it to indent this code up to the #endif?

You can exclude lines of code from being formatted by surrounding them with:

// clang-format off ... // clang-format on

There are examples of this in the generated files.

MarkCallow · 2024-06-19T07:35:35Z

tests/loadtests/geom/cube.h

@@ -3,70 +3,47 @@
 * SPDX-License-Identifier: Apache-2.0


Either don't format this file or fix the formatting for it so the vertices remain separated in the cube face - the formatting has made the comments nonsensical plus it is much harder to see the vertices when they are all run together.

The extra spaces elsewhere are also very helpful for human parsing of the various elements.

There is no way to exclude individual files from formatting. That's why we had to move everything external that is not expected to be formatted by us into a separate subdirectory where we disabled formatting.

You can exclude lines of code from being formatted (or entire files) by surrounding them with:

// clang-format off ... // clang-format on

There are examples of this in the generated files.

MarkCallow · 2024-06-19T07:39:48Z

tests/loadtests/geom/cube_data.h

@@ -10,146 +10,92 @@
 // Mesh and VertexFormat Data


Also don't format this.

MarkCallow · 2024-06-19T07:40:20Z

tests/loadtests/geom/frame.h

@@ -6,18 +6,6 @@
 /*


MarkCallow · 2024-06-19T07:41:16Z

tests/loadtests/geom/quad.h

@@ -9,26 +9,10 @@
 /*


MarkCallow · 2024-06-19T07:44:17Z

tests/unittests/unittests.cc

-ktx_uint8_t CheckHeader1Test::ktxId[12] = {
-    0xAB, 0x4B, 0x54, 0x58, 0x20, 0x31, 0x31, 0xBB, 0x0D, 0x0A, 0x1A, 0x0A
-};
+ktx_uint8_t CheckHeader1Test::ktxId[12] = {0xAB, 0x4B, 0x54, 0x58, 0x20, 0x31,


Is there a way to fix the format of a few lines in a file? This represents the signature of a KTX v1 file so it much easier for humans to follow as originally written.

If there is, please make the same fix to CheckHeader2Test wherever it is.

You can exclude lines of code from being formatted by surrounding them with:

// clang-format off ... // clang-format on

There are examples of this in the generated files.

MarkCallow · 2024-06-19T07:54:06Z

You need to rebase or merge main again to pick up the workaround for the build issue on GitHub Actions. The latest runner image is only partially fixed.

MarkCallow · 2024-07-02T01:27:41Z

@MathiasMagnus please address the outstanding issues and complete this work.

Also remove .clang-format from self-hosted external dep fmt, because it overrides the global disable under external/

The location and the name of the added file is GitHub-friendly, GitHub's blame annotation will also refer to the original author

MathiasMagnus · 2024-07-03T09:11:37Z

@MarkCallow We created this PR out of courtesy, because this task did not fit in the original project timeline, so we are unable to address all the specific personal styling and tooling requests. I added documentation on the minimal requirement to reduce CI friction and noted how to configure VS Code to not trip CI. Vim is too deep a rabbit a hole and I don't have the time to give anything I document in XCode a test drive.

As for the rest of the outstanding formatting related inquiries: instead of whack-a-mole-ing the formatting issues one-by-one and aligning them to the desired format, I reverted the "Initial formatting" commit and just commit the machinery enabling the enforced formatting.

For now, CI job definition is there, but its invocation is commented out.
The GitHub-friendly git blame ignore file is there with a note but not hash.

Because clang-format can't run on an entire source tree, all the tools drive it file-by-file. Should you want to tweak the .clang-format files to perfection, I used the following one-liner on Windows:

# list folders excluding build and .venv | recurse into all | filter files matching regex | for each of the matches { run clang-format inplace using style files and full path to file }
gci -exc build,.venv | gci -rec | where -prop Name -match '^.*\.((((c|C)(c|pp|xx|\+\+)?$)|((h|H)h?(pp|xx|\+\+)?$))|(ino|pde|proto|cu))$' | % { clang-format.exe -i --style=file $_.FullName }

I leave implementing similar logic in the shell of their choice to the end-user. (The regex I borrowed from the script run by the CI action.)

In summary, all the infrastructure for clang-format is there, please tweak it for your needs as you see fit.

MarkCallow · 2024-07-03T12:05:59Z

I am not happy ...

We created this PR out of courtesy, because this task did not fit in the original project timeline, so we are unable to address all the specific personal styling and tooling requests.

I thought providing a PR with initial styling was part of the contract. How else would you deliver the project work, if not by a PR?

Instead of whack-a-mole-ing the formatting issues one-by-one and aligning them to the desired format, I reverted the "Initial formatting" commit and just commit the machinery enabling the enforced formatting.

There is no whack-a-mole on formatting issues here. The related open conversations are everything. I count 2 format change requests and requests to exclude 4 files from formatting. That is not a huge amount of work.

Please restore the formatting making the small number of requested changes.

I understand that documenting this for vim and xcode is potentially time-consuming, if you don't already have the knowledge. Omitting that part of my request is okay.

aqnuep · 2024-07-03T16:56:24Z

I thought providing a PR with initial styling was part of the contract. How else would you deliver the project work, if not by a PR?

Yes, but the contract ended at the end of March with the understanding that due to the additional requests we ran out of time and budget to do the clang-format change. We simply offered to put together a PR anyway afterwards, out of courtesy, as we had free time for it, but we cannot go back-and-forth for months to make the styling fit anybody's personal preferences.

All the infrastructure work is there. Actual styling is subjective and a usual rabbit-hole. You should easily tweak the configurations for your needs, and continuing this back-and-forth is not something that we can continue doing forever.

aqnuep · 2024-07-03T21:17:27Z

There is no whack-a-mole on formatting issues here. The related open conversations are everything. I count 2 format change requests and requests to exclude 4 files from formatting. That is not a huge amount of work.

Please also consider that rebasing a formatting change is a nightmare. We've undone the one-shot formatting, you can adjust the styling as needed, and re-run clang-format on all C++ files then merge it once you're happy with the output. It should be trivial from this point.

MarkCallow · 2024-07-04T09:14:21Z

the understanding that due to the additional requests we ran out of time and budget to do the clang-format change

Were these "additional requests" related or unrelated to clang-format? If the former what where they?

There is no way to exclude individual files from formatting.

Because clang-format can't run on an entire source tree, all the tools drive it file-by-file.

These statements conflict. What am I missing?

MarkCallow · 2024-07-04T12:44:56Z

There is no way to exclude individual files from formatting.

What about .clang-format-ignore which I've just discovered at https://clang.llvm.org/docs/ClangFormat.html.

aqnuep · 2024-07-04T15:45:22Z

the understanding that due to the additional requests we ran out of time and budget to do the clang-format change

Were these "additional requests" related or unrelated to clang-format? If the former what where they?

There is no way to exclude individual files from formatting.

Because clang-format can't run on an entire source tree, all the tools drive it file-by-file.

These statements conflict. What am I missing?

Edit: accidentally pressed the wrong button.

They do not conflict. clang-format, the CLI tool does run file-by-file (unless you run it with a script or indirectly through git clang-format, which is ran for all changed files), but the style sheets (the .clang-format files) apply to entire directories. You cannot change them file-by-file.

The only way to disable clang-format for a single file to either move it to a separate directory for which you disable the styling in the .clang-format file, or you modify the source to surround the content that is not supposed to be formatted with // clang-format off and // clang format on.

MathiasMagnus force-pushed the clang-format-revisited branch 2 times, most recently from 90ae51d to fcdf9e0 Compare June 4, 2024 15:19

MarkCallow mentioned this pull request Jun 5, 2024

WIP prototype clang-format support #889

Closed

MarkCallow reviewed Jun 5, 2024

View reviewed changes

.github/workflows/windows.yml Outdated Show resolved Hide resolved

MarkCallow reviewed Jun 5, 2024

View reviewed changes

interface/basisu_c_binding/inc/basisu_c_binding.h Outdated Show resolved Hide resolved

MathiasMagnus force-pushed the clang-format-revisited branch 2 times, most recently from 6cda98c to 44bb74a Compare June 7, 2024 10:38

MathiasMagnus requested a review from MarkCallow June 7, 2024 15:29

MarkCallow reviewed Jun 19, 2024

View reviewed changes

MathiasMagnus added 5 commits July 3, 2024 10:09

Add .clang-format files

a130b53

Also remove .clang-format from self-hosted external dep fmt, because it overrides the global disable under external/

Exclude generated format files from enforced formatting

95a88c7

Prepare git-blame to ignore formatting changes

59f218d

The location and the name of the added file is GitHub-friendly, GitHub's blame annotation will also refer to the original author

Prepare .clang-format CI check

f671dda

Document transitioning to enforced formatting checks

bc6a8af

MathiasMagnus force-pushed the clang-format-revisited branch from 44bb74a to bc6a8af Compare July 3, 2024 08:38

aqnuep closed this Jul 4, 2024

aqnuep reopened this Jul 4, 2024

MarkCallow merged commit 110f049 into KhronosGroup:main Jul 9, 2024
32 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add clang-format support #913

Add clang-format support #913

MathiasMagnus commented May 29, 2024

MarkCallow Jun 5, 2024

MathiasMagnus Jun 7, 2024

MarkCallow Jun 19, 2024 •

edited

Loading

MathiasMagnus Jun 19, 2024

MarkCallow Jun 19, 2024

MathiasMagnus Jul 3, 2024

MathiasMagnus commented Jun 7, 2024

MarkCallow Jun 19, 2024

MathiasMagnus Jul 3, 2024

aqnuep Jul 3, 2024

MarkCallow Jun 19, 2024

MarkCallow Jun 19, 2024

aqnuep Jul 3, 2024

MarkCallow Jun 19, 2024 •

edited

Loading

aqnuep Jul 3, 2024

MarkCallow Jun 19, 2024

aqnuep Jul 3, 2024

MarkCallow Jun 19, 2024

aqnuep Jul 3, 2024

MarkCallow Jun 19, 2024

aqnuep Jul 3, 2024

MarkCallow Jun 19, 2024 •

edited

Loading

aqnuep Jul 3, 2024

MarkCallow commented Jun 19, 2024

MarkCallow commented Jul 2, 2024

MathiasMagnus commented Jul 3, 2024

MarkCallow commented Jul 3, 2024

aqnuep commented Jul 3, 2024

aqnuep commented Jul 3, 2024

MarkCallow commented Jul 4, 2024

MarkCallow commented Jul 4, 2024

aqnuep commented Jul 4, 2024 •

edited

Loading

Add clang-format support #913

Add clang-format support #913

Conversation

MathiasMagnus commented May 29, 2024

Notes for the reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkCallow Jun 19, 2024 • edited Loading

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

MathiasMagnus commented Jun 7, 2024

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

MarkCallow Jun 19, 2024 • edited Loading

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

MarkCallow Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkCallow commented Jun 19, 2024

MarkCallow commented Jul 2, 2024

MathiasMagnus commented Jul 3, 2024

MarkCallow commented Jul 3, 2024

aqnuep commented Jul 3, 2024

aqnuep commented Jul 3, 2024

MarkCallow commented Jul 4, 2024

MarkCallow commented Jul 4, 2024

aqnuep commented Jul 4, 2024 • edited Loading

MarkCallow Jun 19, 2024 •

edited

Loading

MarkCallow Jun 19, 2024 •

edited

Loading

MarkCallow Jun 19, 2024 •

edited

Loading

aqnuep commented Jul 4, 2024 •

edited

Loading