| Message ID | 4A754814.50506@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Sun, Aug 2, 2009 at 10:02 AM, Roel Kluin<roel.kluin@gmail.com> wrote: > Check whether index is within bounds before testing the element. The change is correct but: - There are other places in the code with that construct. Even though they wouldn't trigger an overflow, why not fixing them too? - Keep the likely: we are more likely to run out of data in the layers than to exhaust the counter (which is why no overflow was ever triggered, I believe ;-) HTH T-Bone > diff --git a/drivers/parisc/pdc_stable.c b/drivers/parisc/pdc_stable.c > index f9f9a5f..13a64bc 100644 > --- a/drivers/parisc/pdc_stable.c > +++ b/drivers/parisc/pdc_stable.c > @@ -370,7 +370,7 @@ pdcspath_layer_read(struct pdcspath_entry *entry, char *buf) >     if (!i) /* entry is not ready */ >         return -ENODATA; > > -    for (i = 0; devpath->layers[i] && (likely(i < 6)); i++) > +    for (i = 0; i < 6 && devpath->layers[i]; i++) >         out += sprintf(out, "%u ", devpath->layers[i]); > >     out += sprintf(out, "\n"); > -- > To unsubscribe from this list: send the line "unsubscribe linux-parisc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at  http://vger.kernel.org/majordomo-info.html >
On Sun, Aug 02, 2009 at 12:06:59PM +0200, Thibaut VARENE wrote: > On Sun, Aug 2, 2009 at 10:02 AM, Roel Kluin<roel.kluin@gmail.com> wrote: > > Check whether index is within bounds before testing the element. > > The change is correct but: > - There are other places in the code with that construct. Even though > they wouldn't trigger an overflow, why not fixing them too? > - Keep the likely: we are more likely to run out of data in the layers > than to exhaust the counter (which is why no overflow was ever > triggered, I believe ;-) No, lose the likely. It's a for-loop; gcc will do the right thing. (If you think I'm wrong, convince me by showing the disassembly of the compiled code with and without the likely).
On Sun, Aug 2, 2009 at 12:16 PM, Matthew Wilcox<matthew@wil.cx> wrote: > On Sun, Aug 02, 2009 at 12:06:59PM +0200, Thibaut VARENE wrote: >> On Sun, Aug 2, 2009 at 10:02 AM, Roel Kluin<roel.kluin@gmail.com> wrote: >> > Check whether index is within bounds before testing the element. >> >> The change is correct but: >> - There are other places in the code with that construct. Even though >> they wouldn't trigger an overflow, why not fixing them too? >> - Keep the likely: we are more likely to run out of data in the layers >> than to exhaust the counter (which is why no overflow was ever >> triggered, I believe ;-) > > No, lose the likely. Â It's a for-loop; gcc will do the right thing. I stand corrected then. ;-) Thanks willy. T-Bone
On Sun, 2009-08-02 at 10:02 +0200, Roel Kluin wrote: > Check whether index is within bounds before testing the element. > > Signed-off-by: Roel Kluin <roel.kluin@gmail.com> > --- > This also removes the likely, should it be kept? > > diff --git a/drivers/parisc/pdc_stable.c b/drivers/parisc/pdc_stable.c > index f9f9a5f..13a64bc 100644 > --- a/drivers/parisc/pdc_stable.c > +++ b/drivers/parisc/pdc_stable.c > @@ -370,7 +370,7 @@ pdcspath_layer_read(struct pdcspath_entry *entry, char *buf) > if (!i) /* entry is not ready */ > return -ENODATA; > > - for (i = 0; devpath->layers[i] && (likely(i < 6)); i++) > + for (i = 0; i < 6 && devpath->layers[i]; i++) Since all patterns like this (swapping the order of conditions with no side effects in a for loop condition) are basically trivial, shouldn't they be going via Jiri Kosina (trivial tree)? James -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/parisc/pdc_stable.c b/drivers/parisc/pdc_stable.c index f9f9a5f..13a64bc 100644 --- a/drivers/parisc/pdc_stable.c +++ b/drivers/parisc/pdc_stable.c @@ -370,7 +370,7 @@ pdcspath_layer_read(struct pdcspath_entry *entry, char *buf) if (!i) /* entry is not ready */ return -ENODATA; - for (i = 0; devpath->layers[i] && (likely(i < 6)); i++) + for (i = 0; i < 6 && devpath->layers[i]; i++) out += sprintf(out, "%u ", devpath->layers[i]); out += sprintf(out, "\n");
Check whether index is within bounds before testing the element. Signed-off-by: Roel Kluin <roel.kluin@gmail.com> --- This also removes the likely, should it be kept? -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html