On 10/14/2016 05:44 PM, Eric Engestrom wrote:
>> Subject: [PATCH 4/4] i965: Silence unused parameter warnings
>
> How about "remove unused parameters" instead?
> Silencing the warnings is nothing more than a side effect of this
> change, albeit the reason you realised it was needed.
There are already quite a few commits in Mesa with subjects that match
this pattern. I've generally used this when the purpose of the change
is to make the compiler complain less.
Also... not all of the changes in this commit remove the unused
parameter. :)
> (Sorry, I've seen so many "silence warning" commits at $DAYJOB that
> this has become a pet peeve of mine)
>
> One suggestion below, but the series looks good:
> Reviewed-by: Eric Engestrom <e...@engestrom.ch>
>
>
> On Fri, Oct 14, 2016 at 11:59:47AM -0700, Ian Romanick wrote:
>> From: Ian Romanick <ian.d.roman...@intel.com>
>>
>> brw_link.cpp:76:44: warning: unused parameter ‘shader_type’
>> [-Wunused-parameter]
>> gl_shader_stage shader_type,
>> ^
>> brw_nir.c: In function ‘brw_nir_lower_vs_inputs’:
>> brw_nir.c:194:55: warning: unused parameter ‘devinfo’ [-Wunused-parameter]
>> const struct gen_device_info *devinfo,
>> ^
>> brw_vec4_visitor.cpp:914:37: warning: unused parameter ‘sampler’
>> [-Wunused-parameter]
>> uint32_t sampler,
>> ^
>> brw_vec4_visitor.cpp:1146:34: warning: unused parameter ‘stream_id’
>> [-Wunused-parameter]
>> vec4_visitor::gs_emit_vertex(int stream_id)
>> ^
>>
>> Signed-off-by: Ian Romanick <ian.d.roman...@intel.com>
>> ---
>> src/mesa/drivers/dri/i965/brw_link.cpp | 3 +--
>> src/mesa/drivers/dri/i965/brw_nir.c | 1 -
>> src/mesa/drivers/dri/i965/brw_nir.h | 1 -
>> src/mesa/drivers/dri/i965/brw_vec4.cpp | 2 +-
>> src/mesa/drivers/dri/i965/brw_vec4.h | 2 +-
>> src/mesa/drivers/dri/i965/brw_vec4_nir.cpp | 2 +-
>> src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 3 +--
>> 7 files changed, 5 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_link.cpp
>> b/src/mesa/drivers/dri/i965/brw_link.cpp
>> index 02151d6..5ea9773 100644
>> --- a/src/mesa/drivers/dri/i965/brw_link.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_link.cpp
>> @@ -73,7 +73,6 @@ brw_shader_precompile(struct gl_context *ctx,
>>
>> static void
>> brw_lower_packing_builtins(struct brw_context *brw,
>> - gl_shader_stage shader_type,
>> exec_list *ir)
>> {
>> /* Gens < 7 don't have instructions to convert to or from half-precision,
>> @@ -105,7 +104,7 @@ process_glsl_ir(struct brw_context *brw,
>> /* lower_packing_builtins() inserts arithmetic instructions, so it
>> * must precede lower_instructions().
>> */
>> - brw_lower_packing_builtins(brw, shader->Stage, shader->ir);
>> + brw_lower_packing_builtins(brw, shader->ir);
>> do_mat_op_to_vec(shader->ir);
>>
>> unsigned instructions_to_lower = (DIV_TO_MUL_RCP |
>> diff --git a/src/mesa/drivers/dri/i965/brw_nir.c
>> b/src/mesa/drivers/dri/i965/brw_nir.c
>> index 744865b..a935f42 100644
>> --- a/src/mesa/drivers/dri/i965/brw_nir.c
>> +++ b/src/mesa/drivers/dri/i965/brw_nir.c
>> @@ -191,7 +191,6 @@ remap_patch_urb_offsets(nir_block *block, nir_builder *b,
>>
>> void
>> brw_nir_lower_vs_inputs(nir_shader *nir,
>> - const struct gen_device_info *devinfo,
>> bool is_scalar,
>> bool use_legacy_snorm_formula,
>> const uint8_t *vs_attrib_wa_flags)
>> diff --git a/src/mesa/drivers/dri/i965/brw_nir.h
>> b/src/mesa/drivers/dri/i965/brw_nir.h
>> index 425d6ce..aef5c53 100644
>> --- a/src/mesa/drivers/dri/i965/brw_nir.h
>> +++ b/src/mesa/drivers/dri/i965/brw_nir.h
>> @@ -99,7 +99,6 @@ nir_shader *brw_preprocess_nir(const struct brw_compiler
>> *compiler,
>> bool brw_nir_lower_intrinsics(nir_shader *nir,
>> struct brw_stage_prog_data *prog_data);
>> void brw_nir_lower_vs_inputs(nir_shader *nir,
>> - const struct gen_device_info *devinfo,
>> bool is_scalar,
>> bool use_legacy_snorm_formula,
>> const uint8_t *vs_attrib_wa_flags);
>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp
>> b/src/mesa/drivers/dri/i965/brw_vec4.cpp
>> index 6aa9102..362f32b 100644
>> --- a/src/mesa/drivers/dri/i965/brw_vec4.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp
>> @@ -2114,7 +2114,7 @@ brw_compile_vs(const struct brw_compiler *compiler,
>> void *log_data,
>> nir_shader *shader = nir_shader_clone(mem_ctx, src_shader);
>> shader = brw_nir_apply_sampler_key(shader, compiler->devinfo, &key->tex,
>> is_scalar);
>> - brw_nir_lower_vs_inputs(shader, compiler->devinfo, is_scalar,
>> + brw_nir_lower_vs_inputs(shader, is_scalar,
>> use_legacy_snorm_formula,
>> key->gl_attrib_wa_flags);
>> brw_nir_lower_vue_outputs(shader, is_scalar);
>> shader = brw_postprocess_nir(shader, compiler->devinfo, is_scalar);
>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h
>> b/src/mesa/drivers/dri/i965/brw_vec4.h
>> index 1505ba6..62c6007 100644
>> --- a/src/mesa/drivers/dri/i965/brw_vec4.h
>> +++ b/src/mesa/drivers/dri/i965/brw_vec4.h
>> @@ -262,7 +262,7 @@ public:
>> src_reg offset_value,
>> src_reg mcs,
>> uint32_t surface, src_reg surface_reg,
>> - uint32_t sampler, src_reg sampler_reg);
>> + src_reg sampler_reg);
>>
>> src_reg emit_mcs_fetch(const glsl_type *coordinate_type, src_reg
>> coordinate,
>> src_reg surface);
>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>> b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>> index 1d834a4..7b36fca 100644
>> --- a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>> @@ -1967,7 +1967,7 @@ vec4_visitor::nir_emit_texture(nir_tex_instr *instr)
>> shadow_comparitor,
>> lod, lod2, sample_index,
>> constant_offset, offset_value, mcs,
>> - texture, texture_reg, sampler, sampler_reg);
>> + texture, texture_reg, sampler_reg);
>> }
>>
>> void
>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
>> b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
>> index 3e785bc..eca753c 100644
>> --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
>> @@ -909,7 +909,6 @@ vec4_visitor::emit_texture(ir_texture_opcode op,
>> src_reg mcs,
>> uint32_t surface,
>> src_reg surface_reg,
>> - uint32_t sampler,
>> src_reg sampler_reg)
>> {
>> /* The sampler can only meaningfully compute LOD for fragment shader
>> @@ -1141,7 +1140,7 @@ vec4_visitor::emit_gen6_gather_wa(uint8_t wa, dst_reg
>> dst)
>> }
>>
>> void
>> -vec4_visitor::gs_emit_vertex(int stream_id)
>> +vec4_visitor::gs_emit_vertex(int /* stream_id */)
>
> `UNUSED`?
I generally prefer this method in C++ code. With UNUSED you don't get
any notification when you start to use a previously unused parameter.
We previously used (void) casting, and that had the same problem. If
the parameter has no name, you know, without a doubt, when you start
using a parameter that was previously unused.
It also makes it easier to scan all the method implementations and
determine that the parameter is never used in any derived class. Right
now I can do this:
$ grep -r gs_emit_vertex src/mesa/drivers/dri/
src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp:vec4_visitor::gs_emit_vertex(int
/* stream_id */)
src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp:vec4_gs_visitor::gs_emit_vertex(int
stream_id)
src/mesa/drivers/dri/i965/brw_vec4_gs_nir.cpp: gs_emit_vertex(stream_id);
src/mesa/drivers/dri/i965/gen6_gs_visitor.h: virtual void gs_emit_vertex(int
stream_id);
src/mesa/drivers/dri/i965/gen6_gs_visitor.cpp:gen6_gs_visitor::gs_emit_vertex(int
stream_id)
src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.h: virtual void
gs_emit_vertex(int stream_id);
src/mesa/drivers/dri/i965/brw_vec4.h: virtual void gs_emit_vertex(int
stream_id);
Assuming that everyone uses the same technique, I see that stream_id is
almost always used. I don't even have to look at the implementations.
If they were marked UNUSED, it might be incorrect. Someone may have
started using the parameter without removing UNUSED. Ditto for (void)
casting.
>> {
>> unreachable("not reached");
>> }
>> --
>> 2.5.5
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
