[PATCH v2 0/4] MR402: vkd3d-shader: Minor fixes to clangd warnings (clangd).
Some fixes to valid clangd warnings, since I am using it as linter. Also, I am removing enum hlsl_error_level on 4/6, since it doesn't seem to be meant to be used anywhere after 3/6. -- v2: vkd3d-shader/d3dbc: Use D3DSIO_TEXKILL instead of VKD3D_SM1_OP_TEXKILL (clangd). vkd3d-shader/hlsl: Remove enum hlsl_error_level (clangd). vkd3d-shader/tpf: Avoid translations to D3DDECLUSAGE and back (clangd). vkd3d-shader: Remove unnecessary fallthroughs (clangd). https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/402
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/spirv.c | 1 + libs/vkd3d-shader/tpf.c | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 1c8b52e6d..1b58e98e5 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -2466,6 +2466,7 @@ static struct spirv_compiler *spirv_compiler_create(const struct vkd3d_shader_ve default: WARN("Ignoring unrecognised option %#x with value %#x.\n", option->name, option->value); + break; case VKD3D_SHADER_COMPILE_OPTION_API_VERSION: break; diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index c471d1c58..cd6f3ee13 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -1803,10 +1803,11 @@ static bool shader_sm4_read_param(struct vkd3d_shader_sm4_parser *priv, const ui *modifier = VKD3DSPSM_ABSNEG; break; + case VKD3D_SM4_REGISTER_MODIFIER_NONE: + break; + default: FIXME("Unhandled register modifier %#x.\n", m); - /* fall-through */ - case VKD3D_SM4_REGISTER_MODIFIER_NONE: break; } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/402
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/tpf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index cd6f3ee13..876bb8290 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -2729,7 +2729,7 @@ bool hlsl_sm4_usage_from_semantic(struct hlsl_ctx *ctx, const struct hlsl_semant const char *name; bool output; enum vkd3d_shader_type shader_type; - D3DDECLUSAGE usage; + D3D_NAME usage; } semantics[] = { -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/402
From: Francisco Casas <fcasas(a)codeweavers.com> It is only used once for calling hlsl_note(), and it expects an enum vkd3d_shader_log_level values instead. --- libs/vkd3d-shader/hlsl.h | 7 ------- libs/vkd3d-shader/hlsl_codegen.c | 4 ++-- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 59c543c7b..9f1a3fe21 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -864,13 +864,6 @@ struct hlsl_ctx uint32_t found_numthreads : 1; }; -enum hlsl_error_level -{ - HLSL_LEVEL_ERROR = 0, - HLSL_LEVEL_WARNING, - HLSL_LEVEL_NOTE, -}; - struct hlsl_resource_load_params { struct hlsl_type *format; diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 5c816e895..7d17ca8ce 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -249,7 +249,7 @@ static struct hlsl_ir_var *add_semantic_var(struct hlsl_ctx *ctx, struct hlsl_ir { hlsl_error(ctx, loc, VKD3D_SHADER_ERROR_HLSL_INVALID_SEMANTIC, "Output semantic \"%s%u\" is used multiple times.", semantic->name, index); - hlsl_note(ctx, &ext_var->loc, HLSL_LEVEL_ERROR, + hlsl_note(ctx, &ext_var->loc, VKD3D_SHADER_LOG_ERROR, "First use of \"%s%u\" is here.", semantic->name, index); semantic->reported_duplicated_output_next_index = index + 1; } @@ -262,7 +262,7 @@ static struct hlsl_ir_var *add_semantic_var(struct hlsl_ctx *ctx, struct hlsl_ir hlsl_error(ctx, loc, VKD3D_SHADER_ERROR_HLSL_INVALID_SEMANTIC, "Input semantic \"%s%u\" is used multiple times with incompatible types.", semantic->name, index); - hlsl_note(ctx, &ext_var->loc, HLSL_LEVEL_ERROR, + hlsl_note(ctx, &ext_var->loc, VKD3D_SHADER_LOG_ERROR, "First declaration of \"%s%u\" is here.", semantic->name, index); semantic->reported_duplicated_input_incompatible_next_index = index + 1; } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/402
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/d3dbc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/d3dbc.c b/libs/vkd3d-shader/d3dbc.c index 598b75183..67fa32710 100644 --- a/libs/vkd3d-shader/d3dbc.c +++ b/libs/vkd3d-shader/d3dbc.c @@ -2221,7 +2221,7 @@ static void write_sm1_jump(struct hlsl_ctx *ctx, struct vkd3d_bytecode_buffer *b struct sm1_instruction instr = { - .opcode = VKD3D_SM1_OP_TEXKILL, + .opcode = D3DSIO_TEXKILL, .dst.type = D3DSPR_TEMP, .dst.reg = reg->id, -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/402
`Subject: [PATCH 1/6] vkd3d-shader/tpf: Remove unnecessary fallthrough (clangd).` What is the issue here exactly? I don't necessarily have an issue with the change, but I wouldn't say it's an improvement purely on its own merits either.
Nothing really, besides that by default clangd emits a warning for all fall-throughs (that are not annotated in a [rather specific way](https://clang.llvm.org/docs/AttributeReference.html#fallthrough)) so this warning shows in many places, but those in 1/5 and 5/5 can be seamlessly removed. I agree that it is not important at all to go hunting for them, it just seemed like a natural think to do. Anyways, I joined 1/5 and 5/5 so it makes less noise. I can also remove them altogether.
```diff #include "hlsl.h" +#include "vkd3d_shader.h" #include <stdio.h> ``` Is that needed? Doesn't hlsl.h pull in vkd3d_shader_private.h, which pulls in vkd3d_shader.h?
No, it is not needed. I may have added it by mistake.
`Subject: [PATCH 4/6] vkd3d-shader/hlsl: Remove enum hlsl_error_level.`
We might as well merge that with the preceding patch that removes the last users of these.
Done! -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/402#note_48424
Nothing really, besides that by default clangd emits a warning for all fall-throughs (that are not annotated in a [rather specific way](https://clang.llvm.org/docs/AttributeReference.html#fallthrough)) so this warning shows in many places, but those in 1/5 and 5/5 can be seamlessly removed.
This happens also on macOS (where the compiler is based on clang), and is the reason why we have `-Wno-implicit-fallthrough` in `build-mac` for the CI. I would solve it properly rather than reorganize our code around that. It seems that both gcc and clang accept `__attribute__((fallthrough));` as a fall through marker. So I would defined a macro called `VKD3D_FALLTHROUGH` that expands to that on gcc and clang and to nothing on other compilers. Then I'd remove `-Wno-implicit-fallthrough` from `build-mac`. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/402#note_48458
On Thu Oct 12 09:12:11 2023 +0000, Giovanni Mascellani wrote:
Nothing really, besides that by default clangd emits a warning for all fall-throughs (that are not annotated in a [rather specific way](https://clang.llvm.org/docs/AttributeReference.html#fallthrough)) so this warning shows in many places, but those in 1/5 and 5/5 can be seamlessly removed. This happens also on macOS (where the compiler is based on clang), and is the reason why we have `-Wno-implicit-fallthrough` in `build-mac` for the CI. I would solve it properly rather than reorganize our code around that. It seems that both gcc and clang accept `__attribute__((fallthrough));` as a fall through marker. So I would defined a macro called `VKD3D_FALLTHROUGH` that expands to that on gcc and clang and to nothing on other compilers. Then I'd remove `-Wno-implicit-fallthrough` from `build-mac`. I should add, though, that both proposed changes to avoid case fallthrough mostly make sense to me in their own, as they make the code a tad more readable, so I'm accepting the change anyway. My suggestion still stands for the other fallthrough cases in the code (mostly `ir.c`, I think), but doesn't have to be in this MR.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/402#note_48459
This merge request was approved by Giovanni Mascellani. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/402
This happens also on macOS (where the compiler is based on clang), and is the reason why we have `-Wno-implicit-fallthrough` in `build-mac` for the CI. I would solve it properly rather than reorganize our code around that. It seems that both gcc and clang accept `__attribute__((fallthrough));` as a fall through marker. So I would defined a macro called `VKD3D_FALLTHROUGH` that expands to that on gcc and clang and to nothing on other compilers. Then I'd remove `-Wno-implicit-fallthrough` from `build-mac`.
So the history of -Wimplicit-fallthrough in vkd3d is that I was sceptical about introducing it in commit 8b49b6e057e6589e2898746b3f29dcaeb8620672, but we only had a single function that needed adjustment, and the gcc mechanism for doing that was innocuous enough. I don't think that's true for the Clang mechanism, and adding a macro for it doesn't help much, in my opinion. My preferred way of addressing this issue would be to simply get rid of -Wimplicit-fallthrough from configure.ac. That said, patch 1/4 would be fine with me if we also moved the default case in spirv_compiler_create() to the bottom of the switch. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/402#note_48494
So the history of -Wimplicit-fallthrough in vkd3d is that I was sceptical about introducing it in commit 8b49b6e057e6589e2898746b3f29dcaeb8620672, but we only had a single function that needed adjustment, and the gcc mechanism for doing that was innocuous enough. I don't think that's true for the Clang mechanism, and adding a macro for it doesn't help much, in my opinion. My preferred way of addressing this issue would be to simply get rid of -Wimplicit-fallthrough from configure.ac.
I think `-Wimplicit-fallback` has some value, but I can live without it. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/402#note_48499
participants (4)
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)