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).
From: Francisco Casas fcasas@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; }
From: Francisco Casas fcasas@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[] = {
From: Francisco Casas fcasas@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; }
From: Francisco Casas fcasas@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,
`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.
#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!
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`.
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.
This merge request was approved by Giovanni Mascellani.
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.
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.
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)