Re: [PATCH v2 17/17] hw/mips: Convert Malta "ifdef 0"-ed code to comments

[Aleksandar Markovic]

пет, 15. мај 2020. у 09:53 Markus Armbruster <address@hidden> је написао/ла:
>
> Aleksandar Markovic <address@hidden> writes:
>
> > The checkpatch complain about "#ifdef 0". Convert corresponding
> > dead code to comments. In future, these cases could be converted
> > to some no-nonsense logging/tracing.
> >
> > Signed-off-by: Aleksandar Markovic <address@hidden>
> > CC: Philippe Mathieu-Daudé <address@hidden>
> > ---
> >  hw/mips/mips_malta.c | 20 ++++++++++++--------
> >  1 file changed, 12 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> > index e4c4de1b4e..f91fa34b06 100644
> > --- a/hw/mips/mips_malta.c
> > +++ b/hw/mips/mips_malta.c
> > @@ -427,10 +427,12 @@ static uint64_t malta_fpga_read(void *opaque, hwaddr 
> > addr,
> >          break;
> >
> >      default:
> > -#if 0
> > -        printf("malta_fpga_read: Bad register offset 0x" TARGET_FMT_lx 
> > "\n",
> > -               addr);
> > -#endif
> > +/*
> > + * Possible logging:
> > + *
> > + *        printf("malta_fpga_read: Bad register offset 0x" TARGET_FMT_lx 
> > "\n",
> > + *               addr);
> > + */
> >          break;
> >      }
> >      return val;
> > @@ -515,10 +517,12 @@ static void malta_fpga_write(void *opaque, hwaddr 
> > addr,
> >          break;
> >
> >      default:
> > -#if 0
> > -        printf("malta_fpga_write: Bad register offset 0x" TARGET_FMT_lx 
> > "\n",
> > -               addr);
> > -#endif
> > +/*
> > + * Possible logging:
> > + *
> > + *        printf("malta_fpga_write: Bad register offset 0x" TARGET_FMT_lx 
> > "\n",
> > + *               addr);
> > + */
> >          break;
> >      }
> >  }
>
> Please don't.
>
> Checkpatch warns "if this code is redundant consider removing it\n".
>
> If it is redundant, do remove it.
>
> If it is not redundant, do ignore checkpatch's warning, do not abuse
> comments to hide from checkpatch.  We'd rather not have to code up a
> warning for that :)
>
> These two look like they want to be tracepoints.
>

Hi, Markus.

I understood your points. They make sense to me. In hindsight, in
general, we shouldn't try just to silence checkpatch warnings (or, for
that matter, compiler warnings as well), but try to resolve the root
cause, the underlying issue, of the warning. In this case, creating
tracepoints seems to be the right thing to do.

In hindsight too, this was my "quick and dirty" way of getting rid of
two checkpatch warnings.

Thanks for your remarks!

Aleksandar

P. S. The ultimate reason for this patch is that we plan renaming this
file in near future, and want to do it in "checkpatch-warning-free"
manner.