godot icon indicating copy to clipboard operation
godot copied to clipboard

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

Open adamscott opened this issue 1 year ago • 1 comments

  • Introduces the FORCE_SEMICOLON macro to force a semi-colon to be inserted (and bypass clang-format).
    • See https://github.com/godotengine/godot/pull/97934#pullrequestreview-2352373936 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)

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

adamscott avatar Oct 07 '24 15:10 adamscott

See also:

  • https://github.com/godotengine/godot/pull/97241

For a different approach (which doesn't apply to CI but allows us to fix this)

AThousandShips avatar Oct 07 '24 15:10 AThousandShips

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

adamscott avatar Oct 22 '24 23:10 adamscott

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

https://github.com/godotengine/godot/blob/6732a0fd867f40751c53f8ed7a3a15bf1b45323f/drivers/metal/rendering_device_driver_metal.mm#L1035-L1043

cc. @stuartcarnie

adamscott avatar Oct 25 '24 12:10 adamscott

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;

Repiteo avatar Oct 25 '24 17:10 Repiteo

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.

clayjohn avatar Oct 25 '24 17:10 clayjohn

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

adamscott avatar Oct 25 '24 17:10 adamscott

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.

adamscott avatar Oct 25 '24 18:10 adamscott

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

Repiteo avatar Oct 25 '24 18:10 Repiteo

Can't really do, the rules are applied from the root to the files below.

adamscott avatar Oct 25 '24 19:10 adamscott

If we're talking about the main file, then yes; but I'm talking about the .glsl fallback file. It should still work after moving /.clang-format-glsl to misc/utility/.clang-format-glsl if you change the pre-commit argument from ["-style=file:.clang-format-glsl"] to ["-style=file:misc/utility/.clang-format-glsl"]

Repiteo avatar Oct 25 '24 19:10 Repiteo

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

https://github.com/godotengine/godot/blob/6732a0fd867f40751c53f8ed7a3a15bf1b45323f/drivers/metal/rendering_device_driver_metal.mm#L1035-L1043

cc. @stuartcarnie

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

stuartcarnie avatar Oct 25 '24 19:10 stuartcarnie

I still wonder how we can compile this using the C++17 standard. 🤔 https://github.com/godotengine/godot/blob/6732a0fd867f40751c53f8ed7a3a15bf1b45323f/drivers/metal/rendering_device_driver_metal.mm#L1035-L1043

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.

https://github.com/godotengine/godot/blob/61accf060515416da07d913580419fd8c8490f7b/drivers/metal/SCsub#L37-L40

adamscott avatar Oct 25 '24 19:10 adamscott

Thanks!

Repiteo avatar Oct 30 '24 00:10 Repiteo

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"

clayjohn avatar Nov 05 '24 01:11 clayjohn