This may be a matter of taste. I personally prefer to give variables a restricted scope, as I find this aids clarity, and -Wshadow can help prevent mistakes from this.
From: Zebediah Figura zfigura@codeweavers.com
Found with -Wshadow. --- libs/vkd3d-shader/hlsl_codegen.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 4a6221857..6a7545da4 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -1090,7 +1090,7 @@ static bool lower_index_loads(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, unsigned int dim_count = hlsl_sampler_dim_count(val->data_type->sampler_dim); struct hlsl_ir_node *coords = index->idx.node; struct hlsl_resource_load_params params = {0}; - struct hlsl_ir_node *load; + struct hlsl_ir_node *resource_load;
assert(coords->data_type->class == HLSL_CLASS_VECTOR); assert(coords->data_type->base_type == HLSL_TYPE_UINT); @@ -1104,9 +1104,9 @@ static bool lower_index_loads(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, params.coords = coords; params.format = val->data_type->e.resource_format;
- if (!(load = hlsl_new_resource_load(ctx, ¶ms, &instr->loc))) + if (!(resource_load = hlsl_new_resource_load(ctx, ¶ms, &instr->loc))) return false; - hlsl_block_add_instr(block, load); + hlsl_block_add_instr(block, resource_load); return true; }
From: Zebediah Figura zfigura@codeweavers.com
Found with -Wshadow. --- tests/d3d12.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/d3d12.c b/tests/d3d12.c index 91e916088..49e3a8408 100644 --- a/tests/d3d12.c +++ b/tests/d3d12.c @@ -19893,7 +19893,7 @@ static void test_get_copyable_footprints(void) uint64_t row_sizes[10], total_size; D3D12_RESOURCE_DESC resource_desc; unsigned int sub_resource_count; - unsigned int i, j, k, l; + unsigned int i, j, k; ID3D12Device *device; UINT row_counts[10]; ULONG refcount; @@ -20090,7 +20090,7 @@ static void test_get_copyable_footprints(void) check_copyable_footprints(&resource_desc, 0, sub_resource_count, base_offsets[k], NULL, NULL, NULL, &total_size);
- for (l = 0; l < sub_resource_count; ++l) + for (unsigned int l = 0; l < sub_resource_count; ++l) { vkd3d_test_push_context("sub-resource %u", l);
From: Zebediah Figura zfigura@codeweavers.com
--- libs/vkd3d-shader/d3dbc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libs/vkd3d-shader/d3dbc.c b/libs/vkd3d-shader/d3dbc.c index 3d139416b..c78ffebac 100644 --- a/libs/vkd3d-shader/d3dbc.c +++ b/libs/vkd3d-shader/d3dbc.c @@ -2253,7 +2253,7 @@ static void write_sm1_jump(struct hlsl_ctx *ctx, struct vkd3d_bytecode_buffer *b { struct hlsl_reg *reg = &jump->condition.node->reg;
- struct sm1_instruction instr = + struct sm1_instruction sm1_instr = { .opcode = D3DSIO_TEXKILL,
@@ -2263,7 +2263,7 @@ static void write_sm1_jump(struct hlsl_ctx *ctx, struct vkd3d_bytecode_buffer *b .has_dst = 1, };
- write_sm1_instruction(ctx, buffer, &instr); + write_sm1_instruction(ctx, buffer, &sm1_instr); break; }
From: Zebediah Figura zfigura@codeweavers.com
--- libs/vkd3d-shader/hlsl.y | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 0dde4c185..d0210b82e 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -487,27 +487,27 @@ static void resolve_loop_continue(struct hlsl_ctx *ctx, struct hlsl_block *block else if (instr->type == HLSL_IR_JUMP) { struct hlsl_ir_jump *jump = hlsl_ir_jump(instr); - struct hlsl_block block; + struct hlsl_block cond_block;
if (jump->type != HLSL_IR_JUMP_UNRESOLVED_CONTINUE) continue;
if (type == LOOP_DO_WHILE) { - if (!hlsl_clone_block(ctx, &block, cond)) + if (!hlsl_clone_block(ctx, &cond_block, cond)) return; - if (!append_conditional_break(ctx, &block)) + if (!append_conditional_break(ctx, &cond_block)) { - hlsl_block_cleanup(&block); + hlsl_block_cleanup(&cond_block); return; } - list_move_before(&instr->entry, &block.instrs); + list_move_before(&instr->entry, &cond_block.instrs); } else if (type == LOOP_FOR) { - if (!hlsl_clone_block(ctx, &block, iter)) + if (!hlsl_clone_block(ctx, &cond_block, iter)) return; - list_move_before(&instr->entry, &block.instrs); + list_move_before(&instr->entry, &cond_block.instrs); } jump->type = HLSL_IR_JUMP_CONTINUE; }
From: Zebediah Figura zfigura@codeweavers.com
--- libs/vkd3d-shader/hlsl.y | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index d0210b82e..ed053f163 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -3553,7 +3553,7 @@ static bool intrinsic_tex(struct hlsl_ctx *ctx, const struct parse_initializer * { struct hlsl_resource_load_params load_params = { 0 }; const struct hlsl_type *sampler_type; - struct hlsl_ir_node *coords, *load; + struct hlsl_ir_node *coords, *sample;
if (params->args_count != 2 && params->args_count != 4) { @@ -3688,9 +3688,9 @@ static bool intrinsic_tex(struct hlsl_ctx *ctx, const struct parse_initializer * load_params.format = hlsl_get_vector_type(ctx, HLSL_TYPE_FLOAT, 4); load_params.sampling_dim = dim;
- if (!(load = hlsl_new_resource_load(ctx, &load_params, loc))) + if (!(sample = hlsl_new_resource_load(ctx, &load_params, loc))) return false; - hlsl_block_add_instr(params->instrs, load); + hlsl_block_add_instr(params->instrs, sample); return true; }
From: Zebediah Figura zfigura@codeweavers.com
--- libs/vkd3d-shader/hlsl_codegen.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 6a7545da4..88cbef61d 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -2301,7 +2301,6 @@ static bool normalize_switch_cases(struct hlsl_ctx *ctx, struct hlsl_ir_node *in struct hlsl_ir_switch_case *c, *def = NULL; bool missing_terminal_break = false; struct hlsl_ir_node *node; - struct hlsl_ir_jump *jump; struct hlsl_ir_switch *s;
if (instr->type != HLSL_IR_SWITCH) @@ -2320,10 +2319,7 @@ static bool normalize_switch_cases(struct hlsl_ctx *ctx, struct hlsl_ir_node *in { node = LIST_ENTRY(list_tail(&c->body.instrs), struct hlsl_ir_node, entry); if (node->type == HLSL_IR_JUMP) - { - jump = hlsl_ir_jump(node); - terminal_break = jump->type == HLSL_IR_JUMP_BREAK; - } + terminal_break = (hlsl_ir_jump(node)->type == HLSL_IR_JUMP_BREAK); }
missing_terminal_break |= !terminal_break;
From: Zebediah Figura zfigura@codeweavers.com
Avoid shadowing "info" in vkd3d_shader_scan_combined_sampler_declaration(). --- libs/vkd3d-shader/vkd3d_shader_main.c | 43 +++++++++++++-------------- 1 file changed, 20 insertions(+), 23 deletions(-)
diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index 77164c9c9..88155502e 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -890,6 +890,21 @@ static void vkd3d_shader_scan_combined_sampler_declaration( &semantic->resource.range, semantic->resource_type, VKD3D_SHADER_RESOURCE_DATA_FLOAT); }
+static const struct vkd3d_shader_descriptor_info1 *find_descriptor( + const struct vkd3d_shader_scan_descriptor_info1 *info, + enum vkd3d_shader_descriptor_type type, unsigned int register_id) +{ + for (unsigned int i = 0; i < info->descriptor_count; ++i) + { + const struct vkd3d_shader_descriptor_info1 *d = &info->descriptors[i]; + + if (d->type == type && d->register_id == register_id) + return d; + } + + return NULL; +} + static void vkd3d_shader_scan_combined_sampler_usage(struct vkd3d_shader_scan_context *context, const struct vkd3d_shader_register *resource, const struct vkd3d_shader_register *sampler) { @@ -915,7 +930,6 @@ static void vkd3d_shader_scan_combined_sampler_usage(struct vkd3d_shader_scan_co
if (vkd3d_shader_ver_ge(context->version, 5, 1)) { - const struct vkd3d_shader_scan_descriptor_info1 *info = context->scan_descriptor_info; const struct vkd3d_shader_descriptor_info1 *d; bool dynamic_resource, dynamic_sampler;
@@ -930,30 +944,13 @@ static void vkd3d_shader_scan_combined_sampler_usage(struct vkd3d_shader_scan_co if (dynamic_resource || dynamic_sampler) return;
- for (i = 0; i < info->descriptor_count; ++i) - { - d = &info->descriptors[i]; - if (d->type != VKD3D_SHADER_DESCRIPTOR_TYPE_SRV) - continue; - if (d->register_id != resource->idx[0].offset) - continue; + if ((d = find_descriptor(context->scan_descriptor_info, + VKD3D_SHADER_DESCRIPTOR_TYPE_SRV, resource->idx[0].offset))) resource_space = d->register_space; - break; - }
- if (sampler) - { - for (i = 0; i < info->descriptor_count; ++i) - { - d = &info->descriptors[i]; - if (d->type != VKD3D_SHADER_DESCRIPTOR_TYPE_SAMPLER) - continue; - if (d->register_id != sampler->idx[0].offset) - continue; - sampler_space = d->register_space; - break; - } - } + if (sampler && (d = find_descriptor(context->scan_descriptor_info, + VKD3D_SHADER_DESCRIPTOR_TYPE_SAMPLER, sampler->idx[0].offset))) + sampler_space = d->register_space; }
for (i = 0; i < info->combined_sampler_count; ++i)
From: Zebediah Figura zfigura@codeweavers.com
--- configure.ac | 1 + 1 file changed, 1 insertion(+)
diff --git a/configure.ac b/configure.ac index 6cd9d9b77..72530a5c0 100644 --- a/configure.ac +++ b/configure.ac @@ -62,6 +62,7 @@ AS_IF([test "x${GCC}" = "xyes"], VKD3D_CHECK_CFLAGS([-Wimplicit-fallthrough]) VKD3D_CHECK_CFLAGS([-Winit-self]) VKD3D_CHECK_CFLAGS([-Wmissing-prototypes]) + VKD3D_CHECK_CFLAGS([-Wshadow]) VKD3D_CHECK_CFLAGS([-Wunused-but-set-parameter]) VKD3D_CHECK_CFLAGS([-Wvla]) VKD3D_CHECK_CFLAGS([-Wpointer-arith])
Giovanni Mascellani (@giomasce) commented about tests/d3d12.c:
check_copyable_footprints(&resource_desc, 0, sub_resource_count, base_offsets[k], NULL, NULL, NULL, &total_size);
for (l = 0; l < sub_resource_count; ++l)
for (unsigned int l = 0; l < sub_resource_count; ++l)
I always thought we don't like to declare variables in the `for` header, and while I don't necessarily agree with that rule I think that there is some rule in keeping things consistent.
I like the MR overall.
I always thought we don't like to declare variables in the `for` header, and while I don't necessarily agree with that rule I think that there is some rule in keeping things consistent.
I think it was only ever forbidden because compilers choked on it, but I think compilers only ever choked on it because we had bugs in Wine preventing -std=c99 from working correctly, which have since been fixed.
I personally prefer it, myself, and in general I prefer limiting variable scopes to the blocks where they're used. I believe Henri tends to have an opposite preference, but so far I don't think he's rejected patches on those grounds.
On Tue Dec 5 10:02:49 2023 +0000, Zebediah Figura wrote:
I always thought we don't like to declare variables in the `for`
header, and while I don't necessarily agree with that rule I think that there is some rule in keeping things consistent. I think it was only ever forbidden because compilers choked on it, but I think compilers only ever choked on it because we had bugs in Wine preventing -std=c99 from working correctly, which have since been fixed. I personally prefer it, myself, and in general I prefer limiting variable scopes to the blocks where they're used. I believe Henri tends to have an opposite preference, but so far I don't think he's rejected patches on those grounds.
Ok, I wasn't aware of that. Well, I like restricting the scope of variables as much as possible too, and actually I prefer to declare variables in code, so that the typing information is as close as possible to the usage point. I was just puzzled by what came to me as a change.
This merge request was approved by Giovanni Mascellani.
I personally prefer it, myself, and in general I prefer limiting variable scopes to the blocks where they're used. I believe Henri tends to have an opposite preference, but so far I don't think he's rejected patches on those grounds.
I'm not sure I'd call it the opposite, but I do like to keep declarations together instead of spreading them around, yes. I also tend to think that for reasonably sized functions it doesn't make much of a difference though...
This merge request was approved by Henri Verbeet.
-
Giovanni Mascellani (＠giomasce) -
Henri Verbeet (＠hverbeet)
-
Zebediah Figura
-
Zebediah Figura (＠zfigura)