Re: [PATCH 1/2] hw/dma: sifive_pdma: Improve code readability for "!!foo & bar"

[Bin Meng]

Hi Philippe,

On Mon, Sep 27, 2021 at 12:47 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 9/27/21 04:21, Bin Meng wrote:
> > GCC seems to be strict about processing pattern like "!!for & bar".
> > When 'bar' is not 0 or 1, it complains with -Werror=parentheses:
> >
> >   suggest parentheses around operand of ‘!’ or change ‘&’ to ‘&&’ or ‘!’ to 
> > ‘~’ [-Werror=parentheses]
> >
> > Add a () around "foo && bar", which also improves code readability.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> >  hw/dma/sifive_pdma.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/dma/sifive_pdma.c b/hw/dma/sifive_pdma.c
> > index b4fd40573a..b8ec7621f3 100644
> > --- a/hw/dma/sifive_pdma.c
> > +++ b/hw/dma/sifive_pdma.c
> > @@ -243,7 +243,7 @@ static void sifive_pdma_write(void *opaque, hwaddr 
> > offset,
> >      offset &= 0xfff;
> >      switch (offset) {
> >      case DMA_CONTROL:
> > -        claimed = !!s->chan[ch].control & CONTROL_CLAIM;
> > +        claimed = !!(s->chan[ch].control & CONTROL_CLAIM);
>
> AFAIK in C logical NOT has precedence over bitwise AND, so IIUC
> compilers should read the current code as:
>
>            claimed (!!s->chan[ch].control) & CONTROL_CLAIM;
>
> meaning this patch is doing more than "improve code readability",
> this is a logical change and likely a bug fix...

Yes, you are correct. Indeed this is a bug fix. I did not dig into the
operator precedence in detail. I will reword this in v2.

>
> BTW GCC suggestions are:
>
>            claimed (!!s->chan[ch].control) & CONTROL_CLAIM;
>
>            claimed (!!s->chan[ch].control) && CONTROL_CLAIM;
> >
> >          if (!claimed && (value & CONTROL_CLAIM)) {
> >              /* reset Next* registers */
> >

I was using GCC 9.3.0 on Ubuntu 20.04.

Regards,
Bin