Re: [PATCH] isdn: hfc_{pci,sx}: Avoid empty body if statements and use proper register accessors

From: Nathan Chancellor <natechancellor@gmail.com>
To: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: isdn@linux-pingi.de, Networking <netdev@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] isdn: hfc_{pci,sx}: Avoid empty body if statements and use proper register accessors
Date: Wed, 17 Oct 2018 20:34:35 -0700	[thread overview]
Message-ID: <20181018033435.GA2734@flashbox> (raw)
In-Reply-To: <CAK7LNARjwZr2NPNE9aGnzQv-uUWjf1=A+kX3pjALqYfsWORv5g@mail.gmail.com>

On Thu, Oct 18, 2018 at 12:23:37PM +0900, Masahiro Yamada wrote:
> Hi Nathan,
> 
> On Thu, Oct 18, 2018 at 3:09 AM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > Clang warns:
> >
> > drivers/isdn/hisax/hfc_pci.c:131:34: error: if statement has empty body
> > [-Werror,-Wempty-body]
> >         if (Read_hfc(cs, HFCPCI_INT_S1));
> >                                         ^
> > drivers/isdn/hisax/hfc_pci.c:131:34: note: put the semicolon on a
> > separate line to silence this warning
> >
> > Use the format found in drivers/isdn/hardware/mISDN/hfcpci.c of casting
> > the return of Read_hfc to void, instead of using an empty if statement.
> >
> > While we're at it, Masahiro Yamada pointed out that {Read,Write}_hfc
> > should be using a standard access method in hfc_pci.h. Use the one found
> > in drivers/isdn/hardware/mISDN/hfc_pci.h.
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/66
> > Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> >  drivers/isdn/hisax/hfc_pci.c | 6 +++---
> >  drivers/isdn/hisax/hfc_pci.h | 4 ++--
> >  drivers/isdn/hisax/hfc_sx.c  | 6 +++---
> >  3 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/isdn/hisax/hfc_pci.c b/drivers/isdn/hisax/hfc_pci.c
> > index 8e5b03161b2f..a63b9155b697 100644
> > --- a/drivers/isdn/hisax/hfc_pci.c
> > +++ b/drivers/isdn/hisax/hfc_pci.c
> > @@ -128,7 +128,7 @@ reset_hfcpci(struct IsdnCardState *cs)
> >         Write_hfc(cs, HFCPCI_INT_M1, cs->hw.hfcpci.int_m1);
> >
> >         /* Clear already pending ints */
> > -       if (Read_hfc(cs, HFCPCI_INT_S1));
> > +       (void) Read_hfc(cs, HFCPCI_INT_S1);
> 
> 
> 
> Why is this '(void)' necessary?
> 

It's not. I never tested without it since last time I tried to remove
the if statement, GCC complained at me but that was before the readb
change like you suggested.

I will go ahead and send a v2 with that cleaned up.

> I see no warning without it.
> 
> 
> 
> -- 
> Best Regards
> Masahiro Yamada

Thanks for the review!
Nathan