3 ms·

I'm not buying these conclusions - on a quick scan, the "strange expression" isn't wrong: the code is scanning to find the first character that isn't a colon. T

by al2o3cr 13y ago

I'm not buying these conclusions - on a quick scan, the "strange expression" isn't wrong: the code is scanning to find the first character that isn't a colon. The check against the zero byte is harmless, and I could imagine other static analysis tools insisting on it (as cheap insurance against scanning off-the-end). As for the "shifting negative integers is undefined behavior", this is a technically-true statement that's been practically-false for quite a few years - does anybody still use a machine that doesn't use two's-complement integers?

JoachimSchipper 13y ago
The strange expression does seem wrong to me: the loop should terminate on end-of-line or ":", so continue while not end-of-line and not ":". I think you're right that shifting negative integers is fine on most any platform you're likely to encounter, but until recently one could also assume that adding two signed integers would wrap in practice, despite being undefined in theory; due to gcc optimizing ever more aggressively, code assuming that signed integers wrap is now broken (see e.g. http://thiemonagel.de/2010/01/signed-integer-overflow/ http://thiemonagel.de/2010/01/signed-integer-overflow/). So I'd be careful with shifts of negative integers in my own code.
- jibsen 13y ago
  It's skipping over ':' characters (which serve as delimiters) to get to the next token of the string (or end of line). So it looks correct, but the check for end of line is superfluous (unless any architecture uses ':' as end of line marker of course). I think it is more likely it looks the way it does because it was made by copy-pasting and editing the loop a few lines further down that copies a token: while (categoryvalue[0] != '\0' && categoryvalue[0] != ':') *cp++ = *categoryvalue++; As for the shifting, I would assume glibc is free to expect whatever holds true on the platforms it currently supports. But fair enough warning about it in a analysis tool.

by al2o3cr 13y ago

JoachimSchipper 13y ago

The strange expression does seem wrong to me: the loop should terminate on end-of-line or ":", so continue while not end-of-line and not ":". I think you're right that shifting negative integers is fine on most any platform you're likely to encounter, but until recently one could also assume that adding two signed integers would wrap in practice, despite being undefined in theory; due to gcc optimizing ever more aggressively, code assuming that signed integers wrap is now broken (see e.g. http://thiemonagel.de/2010/01/signed-integer-overflow/ http://thiemonagel.de/2010/01/signed-integer-overflow/). So I'd be careful with shifts of negative integers in my own code.

jibsen 13y ago
It's skipping over ':' characters (which serve as delimiters) to get to the next token of the string (or end of line). So it looks correct, but the check for end of line is superfluous (unless any architecture uses ':' as end of line marker of course). I think it is more likely it looks the way it does because it was made by copy-pasting and editing the loop a few lines further down that copies a token: while (categoryvalue[0] != '\0' && categoryvalue[0] != ':') *cp++ = *categoryvalue++; As for the shifting, I would assume glibc is free to expect whatever holds true on the platforms it currently supports. But fair enough warning about it in a analysis tool.