[Codestyle] Set clang-format `RemoveSemicolon` rule to `true` #97934

adamscott · 2024-10-07T15:05:16Z

~~Introduces the FORCE_SEMICOLON macro to force a semi-colon to be inserted (and bypass clang-format).~~ (been replaced by turning off and on the checks for the sole exception in the codebase), as mentioned here: [Codestyle] Set clang-format RemoveSemicolon rule to true #97934 (comment))
- ~~See [Codestyle] Set clang-format RemoveSemicolon rule to true #97934 (review) for an example.~~
Sets clang-format Standard rule to c++20
- (as there's currently a c++20 feature in the codebase that messes up with semi-colons otherwise)
adds clang-format-glsl check, that is the same as the usual clang-format, but with certain rules set to keep compatibility.

This should fix the superfluous semi-colons issue that takes a lot of time for some reviewers to flag in PRs.

adamscott · 2024-10-22T23:40:47Z

@Repiteo I updated the PR, removed FORCE_SEMICOLON, and added the commit to .git-blame-ignore-revs.

adamscott · 2024-10-25T12:53:56Z

I still wonder how we can compile this using the C++17 standard. 🤔

godot/drivers/metal/rendering_device_driver_metal.mm

Lines 1035 to 1043 in 6732a0f

    
           template <typename T> 
        
           concept Serializable = requires(T t, BufWriter &p_writer) { 
        
           	{ 
        
           		t.serialize_size() 
        
           	} -> std::same_as<size_t>; 
        
           	{ 
        
           		t.serialize(p_writer) 
        
           	} -> std::same_as<void>; 
        
           };

cc. @stuartcarnie

Repiteo · 2024-10-25T17:31:47Z

Running this across the whole repo now catches some .glsl files in what feels like breaking ways. Can't say for sure, need someone more knowledgeable to confirm/deny (@clayjohn), but this is what the diff looks like:

 drivers/gles3/shaders/canvas_uniforms_inc.glsl | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gles3/shaders/canvas_uniforms_inc.glsl b/drivers/gles3/shaders/canvas_uniforms_inc.glsl
index f6ad2b730a..186f01f3cf 100644
--- a/drivers/gles3/shaders/canvas_uniforms_inc.glsl
+++ b/drivers/gles3/shaders/canvas_uniforms_inc.glsl
@@ -31,7 +31,7 @@
 
 layout(std140) uniform GlobalShaderUniformData { //ubo:1
 	vec4 global_shader_uniforms[MAX_GLOBAL_SHADER_UNIFORMS];
-};
+}
 
 layout(std140) uniform CanvasData { //ubo:0
 	mat4 canvas_transform;
@@ -50,7 +50,7 @@ layout(std140) uniform CanvasData { //ubo:0
 	float tex_to_sdf;
 	uint pad1;
 	uint pad2;
-};
+}
 
 #ifndef DISABLE_LIGHTING
 #define LIGHT_FLAGS_BLEND_MASK uint(3 << 16)
@@ -84,5 +84,5 @@ struct Light {
 
 layout(std140) uniform LightData { //ubo:2
 	Light light_array[MAX_LIGHTS];
-};
+}
 #endif // DISABLE_LIGHTING
 drivers/gles3/shaders/particles.glsl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gles3/shaders/particles.glsl b/drivers/gles3/shaders/particles.glsl
index 2591937a1d..52dd61713e 100644
--- a/drivers/gles3/shaders/particles.glsl
+++ b/drivers/gles3/shaders/particles.glsl
@@ -94,7 +94,7 @@ layout(std140) uniform FrameData { //ubo:0
 
 	Attractor attractors[MAX_ATTRACTORS];
 	Collider colliders[MAX_COLLIDERS];
-};
+}
 
 #define PARTICLE_FLAG_ACTIVE uint(1)
 #define PARTICLE_FLAG_STARTED uint(2)
 drivers/gles3/shaders/scene.glsl | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/gles3/shaders/scene.glsl b/drivers/gles3/shaders/scene.glsl
index fcfbeddb9e..fb8f376afe 100644
--- a/drivers/gles3/shaders/scene.glsl
+++ b/drivers/gles3/shaders/scene.glsl
@@ -153,7 +153,7 @@ layout(location = 15) in highp uvec4 instance_color_custom_data; // Color packed
 
 layout(std140) uniform GlobalShaderUniformData { //ubo:1
 	vec4 global_shader_uniforms[MAX_GLOBAL_SHADER_UNIFORMS];
-};
+}
 
 layout(std140) uniform SceneData { // ubo:2
 	highp mat4 projection_matrix;
@@ -219,7 +219,7 @@ struct PositionalShadowData {
 
 layout(std140) uniform PositionalShadows { // ubo:9
 	PositionalShadowData positional_shadows[MAX_LIGHT_DATA_STRUCTS];
-};
+}
 
 uniform lowp uint positional_shadow_index;
 
@@ -241,7 +241,7 @@ struct DirectionalShadowData {
 
 layout(std140) uniform DirectionalShadows { // ubo:10
 	DirectionalShadowData directional_shadows[MAX_DIRECTIONAL_LIGHT_DATA_STRUCTS];
-};
+}
 
 uniform lowp uint directional_shadow_index;
 
@@ -274,7 +274,7 @@ struct DirectionalLightData {
 
 layout(std140) uniform DirectionalLights { // ubo:7
 	DirectionalLightData directional_lights[MAX_DIRECTIONAL_LIGHT_DATA_STRUCTS];
-};
+}
 #endif // !DISABLE_LIGHT_DIRECTIONAL
 
 // Omni and spot light data.
@@ -302,7 +302,7 @@ struct LightData { // This structure needs to be as packed as possible.
 #if !defined(DISABLE_LIGHT_OMNI) || defined(ADDITIVE_OMNI)
 layout(std140) uniform OmniLightData { // ubo:5
 	LightData omni_lights[MAX_LIGHT_DATA_STRUCTS];
-};
+}
 #ifdef BASE_PASS
 uniform uint omni_light_indices[MAX_FORWARD_LIGHTS];
 uniform uint omni_light_count;
@@ -312,7 +312,7 @@ uniform uint omni_light_count;
 #if !defined(DISABLE_LIGHT_SPOT) || defined(ADDITIVE_SPOT)
 layout(std140) uniform SpotLightData { // ubo:6
 	LightData spot_lights[MAX_LIGHT_DATA_STRUCTS];
-};
+}
 #ifdef BASE_PASS
 uniform uint spot_light_indices[MAX_FORWARD_LIGHTS];
 uniform uint spot_light_count;
@@ -899,7 +899,7 @@ uniform samplerCube refprobe2_texture; // texunit:-8
 
 layout(std140) uniform GlobalShaderUniformData { //ubo:1
 	vec4 global_shader_uniforms[MAX_GLOBAL_SHADER_UNIFORMS];
-};
+}
 
 /* Material Uniforms */
 #ifdef MATERIAL_UNIFORMS_USED
@@ -1011,7 +1011,7 @@ struct DirectionalLightData {
 
 layout(std140) uniform DirectionalLights { // ubo:7
 	DirectionalLightData directional_lights[MAX_DIRECTIONAL_LIGHT_DATA_STRUCTS];
-};
+}
 
 #if defined(USE_ADDITIVE_LIGHTING) && (!defined(ADDITIVE_OMNI) && !defined(ADDITIVE_SPOT))
 // Directional shadows can be in the base pass or in the additive passes
@@ -1045,7 +1045,7 @@ struct LightData { // This structure needs to be as packed as possible.
 #if !defined(DISABLE_LIGHT_OMNI) || defined(ADDITIVE_OMNI)
 layout(std140) uniform OmniLightData { // ubo:5
 	LightData omni_lights[MAX_LIGHT_DATA_STRUCTS];
-};
+}
 #if defined(BASE_PASS) && !defined(USE_VERTEX_LIGHTING)
 uniform uint omni_light_indices[MAX_FORWARD_LIGHTS];
 uniform uint omni_light_count;
@@ -1055,7 +1055,7 @@ uniform uint omni_light_count;
 #if !defined(DISABLE_LIGHT_SPOT) || defined(ADDITIVE_SPOT)
 layout(std140) uniform SpotLightData { // ubo:6
 	LightData spot_lights[MAX_LIGHT_DATA_STRUCTS];
-};
+}
 #if defined(BASE_PASS) && !defined(USE_VERTEX_LIGHTING)
 uniform uint spot_light_indices[MAX_FORWARD_LIGHTS];
 uniform uint spot_light_count;
@@ -1084,7 +1084,7 @@ struct PositionalShadowData {
 
 layout(std140) uniform PositionalShadows { // ubo:9
 	PositionalShadowData positional_shadows[MAX_LIGHT_DATA_STRUCTS];
-};
+}
 
 uniform lowp uint positional_shadow_index;
 #else // ADDITIVE_DIRECTIONAL
@@ -1104,7 +1104,7 @@ struct DirectionalShadowData {
 
 layout(std140) uniform DirectionalShadows { // ubo:10
 	DirectionalShadowData directional_shadows[MAX_DIRECTIONAL_LIGHT_DATA_STRUCTS];
-};
+}
 
 uniform lowp uint directional_shadow_index;
 #endif // !(defined(ADDITIVE_OMNI) || defined(ADDITIVE_SPOT))
 drivers/gles3/shaders/sky.glsl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gles3/shaders/sky.glsl b/drivers/gles3/shaders/sky.glsl
index 186b630bc8..936f19a076 100644
--- a/drivers/gles3/shaders/sky.glsl
+++ b/drivers/gles3/shaders/sky.glsl
@@ -56,7 +56,7 @@ uniform sampler2D quarter_res; //texunit:-3
 
 layout(std140) uniform GlobalShaderUniformData { //ubo:1
 	vec4 global_shader_uniforms[MAX_GLOBAL_SHADER_UNIFORMS];
-};
+}
 
 struct DirectionalLightData {
 	vec4 direction_energy;
 drivers/gles3/shaders/tonemap_inc.glsl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gles3/shaders/tonemap_inc.glsl b/drivers/gles3/shaders/tonemap_inc.glsl
index 6738bdf748..11c8338c07 100644
--- a/drivers/gles3/shaders/tonemap_inc.glsl
+++ b/drivers/gles3/shaders/tonemap_inc.glsl
@@ -8,7 +8,7 @@ layout(std140) uniform TonemapData { //ubo:0
 	float brightness;
 	float contrast;
 	float saturation;
-};
+}
 
 // This expects 0-1 range input.
 vec3 linear_to_srgb(vec3 color) {
 modules/betsy/bc1.glsl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/modules/betsy/bc1.glsl b/modules/betsy/bc1.glsl
index f1b2c28254..3164806606 100644
--- a/modules/betsy/bc1.glsl
+++ b/modules/betsy/bc1.glsl
@@ -17,7 +17,7 @@ layout(binding = 1, rg32ui) uniform restrict writeonly uimage2D dstTexture;
 layout(std430, binding = 2) readonly restrict buffer globalBuffer {
 	float2 c_oMatch5[256];
 	float2 c_oMatch6[256];
-};
+}
 
 layout(push_constant, std430) uniform Params {
 	uint p_numRefinements;
 .../rendering/renderer_rd/shaders/environment/volumetric_fog.glsl   | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/servers/rendering/renderer_rd/shaders/environment/volumetric_fog.glsl b/servers/rendering/renderer_rd/shaders/environment/volumetric_fog.glsl
index 929f1e34df..2a30d6c7ea 100644
--- a/servers/rendering/renderer_rd/shaders/environment/volumetric_fog.glsl
+++ b/servers/rendering/renderer_rd/shaders/environment/volumetric_fog.glsl
@@ -37,7 +37,7 @@ params;
 #ifdef NO_IMAGE_ATOMICS
 layout(set = 1, binding = 1) volatile buffer emissive_only_map_buffer {
 	uint emissive_only_map[];
-};
+}
 #else
 layout(r32ui, set = 1, binding = 1) uniform volatile uimage3D emissive_only_map;
 #endif
@@ -67,10 +67,10 @@ scene_params;
 #ifdef NO_IMAGE_ATOMICS
 layout(set = 1, binding = 3) volatile buffer density_only_map_buffer {
 	uint density_only_map[];
-};
+}
 layout(set = 1, binding = 4) volatile buffer light_only_map_buffer {
 	uint light_only_map[];
-};
+}
 #else
 layout(r32ui, set = 1, binding = 3) uniform volatile uimage3D density_only_map;
 layout(r32ui, set = 1, binding = 4) uniform volatile uimage3D light_only_map;
 .../renderer_rd/shaders/environment/volumetric_fog_process.glsl     | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/servers/rendering/renderer_rd/shaders/environment/volumetric_fog_process.glsl b/servers/rendering/renderer_rd/shaders/environment/volumetric_fog_process.glsl
index 90bbdfe685..22be4a6d9b 100644
--- a/servers/rendering/renderer_rd/shaders/environment/volumetric_fog_process.glsl
+++ b/servers/rendering/renderer_rd/shaders/environment/volumetric_fog_process.glsl
@@ -193,13 +193,13 @@ layout(set = 0, binding = 15) uniform texture3D prev_density_texture;
 #ifdef NO_IMAGE_ATOMICS
 layout(set = 0, binding = 16) buffer density_only_map_buffer {
 	uint density_only_map[];
-};
+}
 layout(set = 0, binding = 17) buffer light_only_map_buffer {
 	uint light_only_map[];
-};
+}
 layout(set = 0, binding = 18) buffer emissive_only_map_buffer {
 	uint emissive_only_map[];
-};
+}
 #else
 layout(r32ui, set = 0, binding = 16) uniform uimage3D density_only_map;
 layout(r32ui, set = 0, binding = 17) uniform uimage3D light_only_map;

clayjohn · 2024-10-25T17:48:40Z

Running this across the whole repo now catches some .glsl files in what feels like breaking ways. Can't say for sure, need someone more knowledgeable to confirm/deny (@clayjohn), but this is what the diff looks like:

Ya, the changes in your diff will break those shaders completely.

adamscott · 2024-10-25T17:49:13Z

I'll investigate, thanks to both of you @clayjohn and @Repiteo !

- Set clang-format `Standard` rule to `c++20`

adamscott · 2024-10-25T18:46:53Z

I added a new pass, clang-format-glsl, that processes .glsl files only using .clang-format-glsl. That file is composed from the same rules as .clang-format, but with the RemoveSemicolon rule turned off.

Technically, .glsl files aren't supported by clang-format, but unsupported files are treated as .cpp files as default. That explains why clang-format can format .glsl files.

That commit just makes it still work, while making it possible to update the rules for other files.

.git-blame-ignore-revs

Repiteo · 2024-10-25T18:58:30Z

I like the workaround, but the file shouldn't be in the project root; migrate it to misc/utility/ instead

stuartcarnie · 2024-10-25T19:28:06Z

I still wonder how we can compile this using the C++17 standard. 🤔

godot/drivers/metal/rendering_device_driver_metal.mm

Lines 1035 to 1043 in 6732a0f

template <typename T>

concept Serializable = requires(T t, BufWriter &p_writer) {

{

t.serialize_size()

} -> std::same_as<size_t>;

{

t.serialize(p_writer)

} -> std::same_as<void>;

};

cc. @stuartcarnie

@adamscott that is in an Objective-C++ file, and only compiled by clang on Apple platforms.

adamscott · 2024-10-25T19:57:58Z

I still wonder how we can compile this using the C++17 standard. 🤔

godot/drivers/metal/rendering_device_driver_metal.mm

Lines 1035 to 1043 in 6732a0f

template <typename T>

concept Serializable = requires(T t, BufWriter &p_writer) {

{

t.serialize_size()

} -> std::same_as<size_t>;

{

t.serialize(p_writer)

} -> std::same_as<void>;

};

cc. @stuartcarnie

@adamscott that is in an Objective-C++ file, and only compiled by clang on Apple platforms.

Maybe that's what you want to say, but it's specifically this part of the SCSub that makes it use C++20 instead of C++17.

godot/drivers/metal/SCsub

Lines 37 to 40 in 61accf0

    
           # Enable C++20 for the Objective-C++ Metal code, which uses C++20 concepts. 
        
           if "-std=gnu++17" in env_metal["CXXFLAGS"]: 
        
               env_metal["CXXFLAGS"].remove("-std=gnu++17") 
        
           env_metal.Append(CXXFLAGS=["-std=c++20"])

Repiteo · 2024-10-30T00:29:44Z

Thanks!

akien-mga · 2024-11-04T18:00:11Z

core/typedefs.h

@@ -315,4 +315,6 @@ struct BuildIndexSequence<0, Is...> : IndexSequence<Is...> {};
 #define ___gd_is_defined(val) ____gd_is_defined(__GDARG_PLACEHOLDER_##val)
 #define GD_IS_DEFINED(x) ___gd_is_defined(x)

+#define FORCE_SEMICOLON ;


You forgot to remove this before merge.

We might want to sneak this into another codebase wise refactoring, as changing typedefs.h again will cause a recompiling of all artifacts for contributors and CI.

We might want to sneak this into another codebase wise refactoring, as changing typedefs.h again will cause a recompiling of all artifacts for contributors and CI.

FYI @Repiteo

Incorporated the macro removal in #93401

You forgot to remove this before merge.

Sorry, that's my fault!

clayjohn · 2024-11-05T01:39:04Z

Just note, this breaks clang formatting for anyone that is using clang format on save instead of through the git pre-commit hook.

I was surprised that all my semicolons were taken away. I'm investigating right now whether I can filter clang_format by file type to get the same kind of workflow that you introduced here.

edit: For VScode it turned out to be very easy! You can specify an alternate style with "clang-format.language.glsl.style"

adamscott added bug topic:codestyle labels Oct 7, 2024

adamscott added this to the 4.4 milestone Oct 7, 2024

adamscott requested review from a team as code owners October 7, 2024 15:05

adamscott removed request for a team October 7, 2024 15:05

adamscott force-pushed the give-AThousandShips-a-break branch from b9c52cc to b1f9f99 Compare October 22, 2024 23:37

Repiteo approved these changes Oct 25, 2024

View reviewed changes

Repiteo removed request for a team October 25, 2024 01:07

AThousandShips approved these changes Oct 25, 2024

View reviewed changes

Set clang-format RemoveSemicolon rule to true

0d350e7

- Set clang-format `Standard` rule to `c++20`

adamscott force-pushed the give-AThousandShips-a-break branch from 3f99de7 to 05a43a3 Compare October 25, 2024 18:42

AThousandShips reviewed Oct 25, 2024

View reviewed changes

.git-blame-ignore-revs Outdated Show resolved Hide resolved

Update .git-blame-ignore-revs for RemoveSemicolon

1671ec7

adamscott force-pushed the give-AThousandShips-a-break branch from 05a43a3 to 2f6bae7 Compare October 25, 2024 18:52

This comment was marked as resolved.

Sign in to view

adamscott force-pushed the give-AThousandShips-a-break branch from 2f6bae7 to 613760f Compare October 25, 2024 19:09

Add .clang-format-glsl check

25b28aa

adamscott force-pushed the give-AThousandShips-a-break branch from 613760f to 25b28aa Compare October 25, 2024 19:11

Repiteo merged commit b7a0971 into godotengine:master Oct 30, 2024
20 checks passed

bruvzg mentioned this pull request Nov 4, 2024

Build error: expected unqualified-id before string constant (mingw32) #98822

Closed

akien-mga reviewed Nov 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Codestyle] Set clang-format `RemoveSemicolon` rule to `true` #97934

[Codestyle] Set clang-format `RemoveSemicolon` rule to `true` #97934

adamscott commented Oct 7, 2024 •

edited

Loading

adamscott commented Oct 22, 2024

adamscott commented Oct 25, 2024

Repiteo commented Oct 25, 2024

clayjohn commented Oct 25, 2024

adamscott commented Oct 25, 2024

adamscott commented Oct 25, 2024 •

edited

Loading

Repiteo commented Oct 25, 2024

This comment was marked as resolved.

This comment was marked as resolved.

stuartcarnie commented Oct 25, 2024

adamscott commented Oct 25, 2024

Repiteo commented Oct 30, 2024

akien-mga Nov 4, 2024

akien-mga Nov 4, 2024

Repiteo Nov 4, 2024

adamscott Nov 4, 2024

clayjohn commented Nov 5, 2024 •

edited

Loading

[Codestyle] Set clang-format RemoveSemicolon rule to true #97934

[Codestyle] Set clang-format RemoveSemicolon rule to true #97934

Conversation

adamscott commented Oct 7, 2024 • edited Loading

adamscott commented Oct 22, 2024

adamscott commented Oct 25, 2024

Repiteo commented Oct 25, 2024

clayjohn commented Oct 25, 2024

adamscott commented Oct 25, 2024

adamscott commented Oct 25, 2024 • edited Loading

Repiteo commented Oct 25, 2024

This comment was marked as resolved.

This comment was marked as resolved.

stuartcarnie commented Oct 25, 2024

adamscott commented Oct 25, 2024

Repiteo commented Oct 30, 2024

akien-mga Nov 4, 2024

Choose a reason for hiding this comment

akien-mga Nov 4, 2024

Choose a reason for hiding this comment

Repiteo Nov 4, 2024

Choose a reason for hiding this comment

adamscott Nov 4, 2024

Choose a reason for hiding this comment

clayjohn commented Nov 5, 2024 • edited Loading

[Codestyle] Set clang-format `RemoveSemicolon` rule to `true` #97934

[Codestyle] Set clang-format `RemoveSemicolon` rule to `true` #97934

adamscott commented Oct 7, 2024 •

edited

Loading

adamscott commented Oct 25, 2024 •

edited

Loading

clayjohn commented Nov 5, 2024 •

edited

Loading