On Friday, 2016-10-14 19:26:38 -0700, Ian Romanick wrote:
> 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. :)
Fair enough :)
>
> > (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.
That's actually a very good point, I never thought about that. I might be
about to make a few changes to our codebase at $DAYJOB following your method.
Thanks!
>
> 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.
Nice trick!
>
> >> {
> >> unreachable("not reached");
> >> }
> >> --
> >> 2.5.5
> >
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
