WIP prototype clang-format support #889

aqnuep · 2024-03-29T13:07:47Z

This is largely WIP at this point.

aqnuep · 2024-04-03T09:41:02Z

@MarkCallow, this is still WIP as I still have to update the auto-generated files and their scripts to include /* clang-format off */ in order to avoid auto-formatting them, and add clang-tidy support. However, this branch already includes the following:

Moving external dependencies to the external directory
Adding a set of clang-format files specific for the directories in question (e.g. lib has a special style file to allow return types on separate lines per the traditional convention)
Running clang-format across all files

Please check the code and let me know whether the new structure and code format seem reasonable to you.

aqnuep · 2024-04-18T08:48:59Z

@MarkCallow, I did not yet get feedback from you about the proposed code formatting conventions applied in this branch.

Should I maybe create a separate PR first that just moves the external components into the external directory (and update build scripts accordingly) to do this step-by-step?

MarkCallow · 2024-04-18T11:04:43Z

@MarkCallow, I did not yet get feedback from you about the proposed code formatting conventions applied in this branch.

Sorry. I am a little under the weather so working when I feel up to it. I reckon reviewing this is going to take some extended concentration so I've been putting it off. Hopefully I'll feel up to taking a look tomorrow.

Should I maybe create a separate PR first that just moves the external components into the external directory (and update build scripts accordingly) to do this step-by-step?

That is not necessary.

I remain somewhat concerned that formatting is done as a workflow rather than via a clean filter run when files are commited. I have to think more about potential ramifications of the remove branch becoming different from what one has just pushed.

Are there any editors that can apply the same formatting during editing?

MarkCallow · 2024-04-18T11:07:04Z

What is the difference between "format" and "tidy"?

aqnuep · 2024-04-18T13:43:55Z

I remain somewhat concerned that formatting is done as a workflow rather than via a clean filter run when files are commited. I have to think more about potential ramifications of the remove branch becoming different from what one has just pushed.

This is the practice used in other Khronos projects: modify, format (automatically or through explicit call), then commit. CI only checks that the modifications match the formatting rules.

Are there any editors that can apply the same formatting during editing?

Many editors have clang-format built-in, AFAICT, but I always do this manually so I don't have much experience with them.

What is the difference between "format" and "tidy"?

clang-format is only dealing with formatting, while clang-tidy is a "linter" and provides higher level analysis, see: https://clang.llvm.org/extra/clang-tidy/

MarkCallow

Here are some comments to start. Some of the comments refer to global things which I've listed here.

1. Narrower line width
2. { on same line as function name. There is no specific comment about this. I want to think about it. I've always put the { on a separate line except on inline functions defined within a class definition. I suppose it is better to be consistent.
3. nesting (indent) of #if and #define
4. case not indented
5. namespace content not indented.

After my experience with this first pass, I think it would be better to make a separate PR for moving all the files. Note there is a comment about also moving etcdec.cxx.

I decided having the actions for format and tidy are fine. I want to investigate providing a clean filter people can install to do the formatting on commit, should they wish, and whether the editors I use can support this formatting during text entry.

MarkCallow · 2024-04-21T07:37:21Z

examples/vkload.cpp

-                                        (getAssetPath() + ktxfile).c_str(),
-                                        KTX_TEXTURE_CREATE_NO_FLAGS,
-                                        &kTexture);
+    ktxresult = ktxTexture_CreateFromNamedFile((getAssetPath() + ktxfile).c_str(), KTX_TEXTURE_CREATE_NO_FLAGS, &kTexture);


I prefer narrower lines because I can read narrower lines faster and I often have at 2 files open side-by-side in Xcode. I see the ColumnLimit is set to 132 in .clang-format. Is that the setting that affects line width? Let's see what it looks like with 80. If that is too little, given the length of some of the token names, we can try 100.

Yes, 132 is the line width. Note that this (and many other rules) are the default ones used across other Khronos repositories. I think we should use at least 100 characters though as 80 is just too short for modern standards.

MarkCallow · 2024-04-21T07:40:43Z

examples/vkload.cpp

-    ktxresult = ktxTexture_VkUploadEx(kTexture, &kvdi, &texture,
-                                      VK_IMAGE_TILING_OPTIMAL,
-                                      VK_IMAGE_USAGE_SAMPLED_BIT,
+    ktxresult = ktxTexture_VkUploadEx(kTexture, &kvdi, &texture, VK_IMAGE_TILING_OPTIMAL, VK_IMAGE_USAGE_SAMPLED_BIT,


Is there a way to get it to put one parameter per line if the number of parameters exceeds some value?

Not sure, I'll have to look into that, but I would be surprised if clang-format wouldn't have an option for such a common style.

MarkCallow · 2024-04-21T07:48:33Z

examples/vkload.cpp

-void
-Texture::prepareSamplerAndView()
-{
+void Texture::prepareSamplerAndView() {


Can you put the type on a separate line in .cpp file or will such a setting affect inline functions in class definitions as well?

Historically putting the name of a function at its definition at the very beginning of a line made for an easy way to find that definition. Probably less important for c++ because you don't often call a fully qualified name, e.g, Texture::prepareSamplerAndView, so a search for that will find the definition.

For both C and c modern IDEs have mechanisms to go to a definition so it is not as important as it used to be.

So this is an on/off option. I used return type on separate line for the libktx code because that was the style, but used return type on same line for newer things like the ktx CLI tools which use more modern style. If you could provide a list of directories where you'd like us to retain the legacy "return type on separate line" style, we can just configure things accordingly.

MarkCallow · 2024-04-21T07:54:03Z

interface/basisu_c_binding/inc/basisu_c_binding.h

@@ -4,36 +4,32 @@
 #pragma once

 #if defined(KHRONOS_STATIC)
-  #define KTX_BASISU_API
+#define KTX_BASISU_API


I guess indenting # is not officially supported but every compiler we use supports it and, as far as I am concerned, it makes nested #ifs far easier to understand and less error prone.

Is it possible to get clang-format to indent them?

I'll look into whether there's a style option for that.

MarkCallow · 2024-04-21T07:56:45Z

interface/basisu_c_binding/src/basisu_c_binding.cpp

-            // 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
+        if (transcoder_format == transcoder_texture_format::cTFPVRTC1_4_RGB ||
+            transcoder_format == transcoder_texture_format::cTFPVRTC1_4_RGBA) {


Is is possible to have it put the operator at the start of the second line instead of the end of the first when making multi-line expressions?

I'll have to look into that too.

MarkCallow · 2024-04-21T08:12:11Z

lib/etcdec.cxx

@@ -1,9 +1,9 @@
 /*


We should move this file to external/etcdec or similar so it is not reformatted. Strict adherence to the terms of the license require not modifying the file.

Thanks for the info, I didn't notice this is also an external file.

MarkCallow · 2024-04-21T08:19:02Z

lib/etcunpack.cxx

+    int dstChannels, dstChannelBytes;
+
+    switch (srcFormat) {
+    case GL_COMPRESSED_SIGNED_R11_EAC:


Can you indent the cases by 2 spaces? Since I'm always fighting editors to do this we probably shouldn't continue doing it but I'm curious.

We'll look into this as well.

MarkCallow · 2024-04-21T08:29:52Z

lib/.clang-format

+# SPDX-License-Identifier: Apache-2.0
+---
+# Use defaults from the Google style with the following exceptions:
+Language: Cpp


Does this apply to the c files as well?

MarkCallow · 2024-04-21T08:36:35Z

lib/gl_funcs.c

@@ -69,21 +69,21 @@ HMODULE ktxOpenGLModuleHandle;
 #define GetOpenGLModuleHandle(flags) dlopen(NULL, flags)
 #define LoadProcAddr dlsym
 #define LIBRARY_NAME NULL
-void* ktxOpenGLModuleHandle;
+void *ktxOpenGLModuleHandle;


I saw the opposite change being made in basisu_c_binding.{cpp,h}, foo *bar became foo* bar. Why the difference here? Is it a c vs C thing? I prefer foo* bar because to my mind the type of "bar" is "pointer to foo".

No, this is one of those configurable things. We can go either way. The reason why this style is the default in the Google style because technically the "*" belongs to the variable name (imagine when multiple variables are declared in a comma separated list like int *a_ptr, b_value, *c_ptr;).

My question here is why the difference in basisu_c_binding.* vs within the libktx code?

That's a good question. Now that you say, I saw similar inconsistencies in clang-format behavior on other Khronos ecosystem repositories. Indeed could be a C vs C++ difference, but may also be something more subtle.

MarkCallow · 2024-04-21T08:44:48Z

lib/vkformat_check.c

@@ -15,72 +15,71 @@
 #include "vkformat_enum.h"

 bool
-isProhibitedFormat(VkFormat format)
-{
+isProhibitedFormat(VkFormat format) {


Has the generator been modified to match the clang format or has clang modified this file? I don't see anything to tell clang to ignore it.

clang-format modified it.

For generated files we really have various options:

Not format them at all (would require moving generated files into a dedicated folder marked to not format)

Format them (by explicitly calling e.g. git clang-format before your commit) - kind of the current proposal

Integrate calling clang-format into the generation process

I thought you were going to modify the generators to add a line that tells clang_format not to touch the file and to output in whatever format we eventually settle on.

How does clang-format get installed on one's system? My concern is for when we eventually move to generating the VkFormat related files from vk.xml in the Vulkan-Docs repo. Will #3 require installing clang-format in that repo.

So it is an option to not auto-format the generated files, but then they also need to be moved to a dedicated directory, because clang-format styles can only be changed (in this case disabled) per directory.

It's up to us whether we want to do that instead, but with this prototype I proposed that it may be best to just format them as other files (and after they are regenerated they are also reformatted using git clang-format, per normal).

but then they also need to be moved to a dedicated directory, because clang-format styles can only be changed (in this case disabled) per directory.

What happened to this?

I still have to update the auto-generated files and their scripts to include /* clang-format off */ in order to avoid auto-formatting them,

Oh you're right. I completely forgot that's what we agreed on with @lexaknyazev.

MarkCallow · 2024-06-05T05:44:33Z

Shall we close this now that #913 exists?

aqnuep · 2024-06-05T08:09:42Z

Yes, closing.

aqnuep force-pushed the clang-format branch 2 times, most recently from 948dc38 to 49f11fb Compare April 3, 2024 08:50

Prototype clang-format support

1181d74

aqnuep force-pushed the clang-format branch from 49f11fb to 1181d74 Compare April 3, 2024 09:04

MarkCallow reviewed Apr 21, 2024

View reviewed changes

MathiasMagnus mentioned this pull request May 14, 2024

Move self-hosted dependencies to external folder #909

Merged

MathiasMagnus mentioned this pull request May 29, 2024

Add clang-format support #913

Open

aqnuep closed this Jun 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP prototype clang-format support #889

WIP prototype clang-format support #889

aqnuep commented Mar 29, 2024

aqnuep commented Apr 3, 2024 •

edited

aqnuep commented Apr 18, 2024

MarkCallow commented Apr 18, 2024

MarkCallow commented Apr 18, 2024

aqnuep commented Apr 18, 2024

MarkCallow left a comment •

edited

MarkCallow Apr 21, 2024

aqnuep Apr 29, 2024

MarkCallow Apr 21, 2024

aqnuep Apr 29, 2024

MarkCallow Apr 21, 2024 •

edited

aqnuep Apr 29, 2024

MarkCallow Apr 21, 2024

aqnuep Apr 29, 2024

MarkCallow Apr 21, 2024

aqnuep Apr 29, 2024

MarkCallow Apr 21, 2024 •

edited

aqnuep Apr 29, 2024

MarkCallow Apr 21, 2024 •

edited

aqnuep Apr 29, 2024

MarkCallow Apr 21, 2024

MarkCallow Apr 21, 2024 •

edited

aqnuep Apr 29, 2024

MarkCallow Apr 29, 2024

aqnuep Apr 29, 2024

MarkCallow Apr 21, 2024

aqnuep Apr 29, 2024

MarkCallow Apr 29, 2024

aqnuep Apr 29, 2024

MarkCallow Apr 29, 2024

aqnuep Apr 30, 2024

MarkCallow commented Jun 5, 2024

aqnuep commented Jun 5, 2024

WIP prototype clang-format support #889

WIP prototype clang-format support #889

Conversation

aqnuep commented Mar 29, 2024

aqnuep commented Apr 3, 2024 • edited

aqnuep commented Apr 18, 2024

MarkCallow commented Apr 18, 2024

MarkCallow commented Apr 18, 2024

aqnuep commented Apr 18, 2024

MarkCallow left a comment • edited

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 Apr 21, 2024 • edited

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 Apr 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkCallow Apr 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkCallow Apr 21, 2024 • edited

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

MarkCallow commented Jun 5, 2024

aqnuep commented Jun 5, 2024

aqnuep commented Apr 3, 2024 •

edited

MarkCallow left a comment •

edited

MarkCallow Apr 21, 2024 •

edited

MarkCallow Apr 21, 2024 •

edited

MarkCallow Apr 21, 2024 •

edited

MarkCallow Apr 21, 2024 •

edited