On Thu, 2017-02-23 at 18:33 -0800, Joe Perches wrote:
> On Thu, 2017-02-23 at 14:17 -0800, Ricardo Neri wrote:
> > On Thu, 2017-02-23 at 08:24 +0100, Peter Zijlstra wrote:
> > > On Wed, Feb 22, 2017 at 10:36:50PM -0800, Ricardo Neri wrote:
> > > > +                       /*
> > > > +                        * A negative offset generally means a error, 
> > > > except
> > > > +                        * -EDOM, which means that the contents of the 
> > > > register
> > > > +                        * should not be used as index.
> > > > +                        */
> > > >                         if (indx_offset < 0)
> > > > -                               goto out_err;
> > > > +                               if (indx_offset == -EDOM)
> > > > +                                       indx = 0;
> > > > +                               else
> > > > +                                       goto out_err;
> > > > +                       else
> > > > +                               indx = regs_get_register(regs, 
> > > > indx_offset);
> > > 
> > > Kernel coding style requires more brackets than are strictly required by
> > > C, any block longer than 1 line needs then. Also, if one leg of a
> > > conditional needs them, then they should be on both legs.
> > > 
> > > Your code has many such instances, please change them all.
> > 
> > Will do. Sorry for the noise. These instances escaped the checkpatch
> > script.
> 
> Also, this code would read better with the inner test
> reversed or done first
> 
>               if (indx_offset < 0) {
>                       if (indx_offset != -EDOM)
>                               goto out_err;
>                       indx = 0;
>               } else {
>                       indx = regs_get_register(etc...)
>               }
> 
> or
>               if (indx_offset == -EDOM)
>                       indx = 0;
>               else if (indx_offset < 0)
>                       goto err;
>               else
>                       indx = regs_get_register(etc...)
> 
> The compiler should generate the same code in any
> case, but either could improve reader understanding.

I agree! I will change it with your clever solution.

Thanks and BR,
Ricardo


--
To unsubscribe from this list: send the line "unsubscribe linux-msdos" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to