<div dir="auto"><div><br><div class="gmail_extra"><br><div class="gmail_quote">On Feb 23, 2017 9:33 PM, "Joe Perches" <<a href="mailto:joe@perches.com">joe@perches.com</a>> wrote:<br type="attribution"><blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="elided-text">On Thu, 2017-02-23 at 14:17 -0800, Ricardo Neri wrote:<br>
> On Thu, 2017-02-23 at 08:24 +0100, Peter Zijlstra wrote:<br>
> > On Wed, Feb 22, 2017 at 10:36:50PM -0800, Ricardo Neri wrote:<br>
> > > +                 /*<br>
> > > +                  * A negative offset generally means a error, except<br>
> > > +                  * -EDOM, which means that the contents of the register<br>
> > > +                  * should not be used as index.<br>
> > > +                  */<br>
> > >                   if (indx_offset < 0)<br>
> > > -                         goto out_err;<br>
> > > +                         if (indx_offset == -EDOM)<br>
> > > +                                 indx = 0;<br>
> > > +                         else<br>
> > > +                                 goto out_err;<br>
> > > +                 else<br>
> > > +                         indx = regs_get_register(regs, indx_offset);<br>
> ><br>
> > Kernel coding style requires more brackets than are strictly required by<br>
> > C, any block longer than 1 line needs then. Also, if one leg of a<br>
> > conditional needs them, then they should be on both legs.<br>
> ><br>
> > Your code has many such instances, please change them all.<br>
><br>
> Will do. Sorry for the noise. These instances escaped the checkpatch<br>
> script.<br>
<br>
</div>Also, this code would read better with the inner test<br>
reversed or done first<br>
<br>
                if (indx_offset < 0) {<br>
                        if (indx_offset != -EDOM)<br>
                                goto out_err;<br>
                        indx = 0;<br>
                } else {<br>
                        indx = regs_get_register(etc...)<br>
                }<br>
<br>
or<br>
                if (indx_offset == -EDOM)<br>
                        indx = 0;<br>
                else if (indx_offset < 0)<br>
                        goto err;<br></blockquote></div></div></div><div dir="auto"><br></div><div dir="auto">Or goto out_err;</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
                else<br>
                        indx = regs_get_register(etc...)<br>
<br>
The compiler should generate the same code in any<br>
case, but either could improve reader understanding.<br>
</blockquote></div><br></div><div class="gmail_extra" dir="auto">Also, it may be a tweak more efficient to handle the most likely runtime case in the conditional stack first (whichever that may be).</div></div></div>