Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI: Update clang-format pre-commit hook to latest version #93313

Merged
merged 3 commits into from
Sep 20, 2024

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Jun 18, 2024

Accompanies #93271

Updates clang-format to the latest available version: 18.1.6. This will largely sync the hook with what is used in IDEs, as those tend to use the latest version when applicable. Additionally modernized the .clang-format file to sync with llvm's settings on the latest version. All uncommented rules remain as they were in the prior version, with the only difference being replacing the now-removed ConstructorInitializerAllOnOneLineOrOnePerLine: true with the modern equivalent PackConstructorInitializers: NextLine.

It seems some more default enforcements have been made since then, as well as general parsing improvements. As such, there's another commit applying clang-format across the repo (only whitespace changes) & excluding it in .git-blame-ignore-revs with a subsequent commit.

@Repiteo Repiteo removed request for a team June 26, 2024 15:52
@Repiteo
Copy link
Contributor Author

Repiteo commented Jun 26, 2024

Decided to expand the scope of this PR slightly to include that aforementioned semicolon fix, as there was fairly significant overlap between the files that would've been affected.

@AThousandShips
Copy link
Member

Upgrading clang-format version might affect various workflows so I think it's something to take a deep look into, I think we shouldn't just focus on the hook but manual use and different possible targets and OSs running it as well

@Repiteo
Copy link
Contributor Author

Repiteo commented Jun 26, 2024

Hmm, that's a fair point. I'll go back to my original idea and make the semicolon fix separate so this can focus on clang-format instead.

@AThousandShips
Copy link
Member

I think doing that separately might be good, though it is a bit of a sweeping change but with blame ignoring it won't create noise so I think it's worth it (though it might create some annoying conflicts)

@Repiteo
Copy link
Contributor Author

Repiteo commented Jun 26, 2024

I believe some form of update should happen regardless to clang-format, as right now the live hook version (v17.0.6) doesn't match the live last-synced version (v14.0). Additionally, the clang-tidy hook on live is modernized at v18.1.1; while not integrated in any workflow atm, I think it's reasonable to have major/minor versions the clang tools synced. Granted, this could be inversely applied by bumping clang-tidy down to 17.x; I'd be fine with that, but still ultimately prefer keeping these tools as updated as possible.

• Modernized `.clang-format` file against latest LLVM config settings
@Repiteo Repiteo force-pushed the ci/clang-format branch 3 times, most recently from 8d2b2bc to beca879 Compare September 19, 2024 21:04
@@ -43,7 +43,7 @@ class EditorTitleBar : public HBoxContainer {

protected:
virtual void gui_input(const Ref<InputEvent> &p_event) override;
static void _bind_methods(){};
static void _bind_methods() {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove these empty _bind_methods? What's the point of overriding the virtual method to do nothing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Post from the past, seems like I had forgotten to send my review...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ehh, I'll leave it be; I'd rather keep all the changes strictly stylistic.

drivers/gles3/storage/render_scene_buffers_gles3.h Outdated Show resolved Hide resolved
Comment on lines 46 to 47
}
material;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had missed that one in my own PR, but that's another new clang-format bug :\

It's always been hit and miss on our custom glsl with bits of invalid syntax we replace at compile time.

I think we've accepted to just live with it, though I wonder why this is now changing when it didn't before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One option if we don't want this to happen is to just disable clang-format around this layout definition:

diff --git a/servers/rendering/renderer_rd/shaders/canvas.glsl b/servers/rendering/renderer_rd/shaders/canvas.glsl
index e64dd17072..fb813c5ab2 100644
--- a/servers/rendering/renderer_rd/shaders/canvas.glsl
+++ b/servers/rendering/renderer_rd/shaders/canvas.glsl
@@ -41,10 +41,13 @@ layout(location = 3) out vec2 pixel_size_interp;
 #endif
 
 #ifdef MATERIAL_UNIFORMS_USED
+/* clang-format off */
 layout(set = 1, binding = 0, std140) uniform MaterialUniforms {
+
 #MATERIAL_UNIFORMS
-}
-material;
+
+} material;
+/* clang-format on */
 #endif
 
 #GLOBALS

We already do that in a number of places in .glsl shaders, e.g. in drivers/gles3/shaders/scene.glsl:

/* clang-format off */
layout(std140) uniform MaterialUniforms { // ubo:3

#MATERIAL_UNIFORMS

};
/* clang-format on */

CC @clayjohn

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll disable clang-format for those blocks. Really wish there was a way to ignore single lines like clang-tidy…

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Sep 20, 2024
@Repiteo Repiteo force-pushed the ci/clang-format branch 2 times, most recently from 96a69d4 to 8389681 Compare September 20, 2024 13:03
@akien-mga akien-mga merged commit da9764a into godotengine:master Sep 20, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

@Repiteo Repiteo deleted the ci/clang-format branch September 20, 2024 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants