[PATCH] Fix a GCC warning: comparing signed vs. unsigned

[Scott Kostyshak]

Is this OK? Note that converting the left hand side to an unsigned int
would work also, because it was checked just above that nRead is >=0.
But I figure converting the signed to unsigned is better in general as
long as the numbers are not expected to be large.

Scott

[Richard Heck]

Post by Scott Kostyshak
Is this OK? Note that converting the left hand side to an unsigned int
would work also, because it was checked just above that nRead is >=0.
But I figure converting the signed to unsigned is better in general as
long as the numbers are not expected to be large.

I would guess the safest thing would be to convert nRead to unsigned,
since (a) we know it
is positive and (b) there's no possible danger of a misconversion, as
there is in the other case.
But maybe someone who knows what they're talking about should chime in.

Richard

[Scott Kostyshak]

Post by Scott Kostyshak
Is this OK? Note that converting the left hand side to an unsigned int
would work also, because it was checked just above that nRead is >=0.
But I figure converting the signed to unsigned is better in general as
long as the numbers are not expected to be large.

I would guess the safest thing would be to convert nRead to unsigned, since
(a) we know it
is positive and (b) there's no possible danger of a misconversion, as there
is in the other case.

Yeah that makes sense too. I was more worried about the 'if' block
being moved above the if block above it, in which case we would no
longer know that nRead is positive. And since the size of 'buf' is
hardcoded to 1024, it seems likely not to go past 2^31-1.

But maybe someone who knows what they're talking about should chime in.

I would also be interested in more opinions.

Scott

[Cyrille Artho]

Post by Scott Kostyshak

Post by Scott Kostyshak
Is this OK? Note that converting the left hand side to an unsigned int
would work also, because it was checked just above that nRead is >=0.
But I figure converting the signed to unsigned is better in general as
long as the numbers are not expected to be large.

I would guess the safest thing would be to convert nRead to unsigned, since
(a) we know it
is positive and (b) there's no possible danger of a misconversion, as there
is in the other case.

Yeah that makes sense too. I was more worried about the 'if' block
being moved above the if block above it, in which case we would no
longer know that nRead is positive. And since the size of 'buf' is
hardcoded to 1024, it seems likely not to go past 2^31-1.

But maybe someone who knows what they're talking about should chime in.

I would also be interested in more opinions.
Scott

I would change variable "nRead" to ssize_t, as it's common that -1 is
used as a return value in case of a read error, and signed/unsignedness
conversions produce potentially dangerous patterns. (Not only the case
where code is moved around, but also if it's copy/pasted.)

You are right that unsigned int may overflow in the far future - hence
use ssize_t, which is the right data type here (at least on Mac OS X,
I'm out of office now so I can't check other platforms).

[Georg Baum]

Post by Cyrille Artho

Post by Scott Kostyshak
Is this OK? Note that converting the left hand side to an unsigned int
would work also, because it was checked just above that nRead is >=0.
But I figure converting the signed to unsigned is better in general as
long as the numbers are not expected to be large.

Please be careful with "never expected to be large". The warning you are
adressing is about a subtle error which usually does not occur, but there is
a small chance that it can occur. When fixing warnings like that I very much
prefer to fix them in a 100% safe way without any assumptions. Otherwise one
could just switch the appropriate warning off.

Post by Cyrille Artho
I would change variable "nRead" to ssize_t, as it's common that -1 is
used as a return value in case of a read error, and signed/unsignedness
conversions produce potentially dangerous patterns. (Not only the case
where code is moved around, but also if it's copy/pasted.)
You are right that unsigned int may overflow in the far future - hence
use ssize_t, which is the right data type here (at least on Mac OS X,
I'm out of office now so I can't check other platforms).

It is the correct type on linux as well. Therefore, the approach suggested
by Cyrille is IMHO the best one: Change the type of nRead to ssize_t (note
the two s), and change the if-condition to

if (static_cast<size_t>(nRead) < buf.size() - 1) {

This is 100% safe: We know that nRead > 0 at the time the cast occurs, and
ssize_t has the same number of bits as size_t, and buf.size() returns
size_t, and buf.size() is never 0. The static_cast is better than a C-style
cast, since you can search for it, and it expresses what is wanted (C-style
casts could also be used to cast constness away etc).


Georg

[Cyrille Artho]

Post by Georg Baum
It is the correct type on linux as well. Therefore, the approach suggested
by Cyrille is IMHO the best one: Change the type of nRead to ssize_t (note
the two s), and change the if-condition to
if (static_cast<size_t>(nRead) < buf.size() - 1) {
This is 100% safe: We know that nRead > 0 at the time the cast occurs, and
ssize_t has the same number of bits as size_t, and buf.size() returns
size_t, and buf.size() is never 0. The static_cast is better than a C-style
cast, since you can search for it, and it expresses what is wanted (C-style
casts could also be used to cast constness away etc).

Georg made a small type: The cast should be

Post by Georg Baum
if (static_cast<ssize_t>(nRead) < buf.size() - 1) {

as mentioned earlier (two s). In general, though, I think it's better to
adapt the variable type such that type casts are not necessary. In this
case, "nRead" would be changed to ssize_t. Without looking at all the code,
I'm not sure if this is the best solution here (sometimes changing one
variable type requires many subsequent changes).

[Georg Baum]

Post by Cyrille Artho

Post by Georg Baum
It is the correct type on linux as well. Therefore, the approach
suggested by Cyrille is IMHO the best one: Change the type of nRead
to ssize_t (note the two s), and change the if-condition to
if (static_cast<size_t>(nRead) < buf.size() - 1) {
This is 100% safe: We know that nRead > 0 at the time the cast occurs,
and ssize_t has the same number of bits as size_t, and buf.size()
returns size_t, and buf.size() is never 0. The static_cast is better
than a C-style cast, since you can search for it, and it expresses
what is wanted (C-style casts could also be used to cast constness
away etc).

Georg made a small type: The cast should be

Post by Georg Baum
if (static_cast<ssize_t>(nRead) < buf.size() - 1) {

as mentioned earlier (two s).

No, there is no typo, I meant size_t with one s. The reason for that is to
avoid the signed vs. unsigned comparison warning, since ssize_t is signed,
and the expression on the right hand side is unsigned. If the cast to
ssize_t was correct then no cast would be needed at all, since nRead is
already of type ssize_t.

Post by Cyrille Artho
In general, though, I think it's better to
adapt the variable type such that type casts are not necessary. In this
case, "nRead" would be changed to ssize_t. Without looking at all the
code, I'm not sure if this is the best solution here (sometimes
changing one variable type requires many subsequent changes).

Yes, the type of nRead should be changed to ssize_t. However, a cast in the
if-clause is still needed to avoid the signed vs. unsigned warning.


Georg

[Scott Kostyshak]

On Mon, May 19, 2014 at 3:09 PM, Georg Baum

Post by Georg Baum
Please be careful with "never expected to be large". The warning you are
adressing is about a subtle error which usually does not occur, but there is
a small chance that it can occur. When fixing warnings like that I very much
prefer to fix them in a 100% safe way without any assumptions.

I see why that was poor thinking. I agree.

Post by Georg Baum
It is the correct type on linux as well. Therefore, the approach suggested
by Cyrille is IMHO the best one: Change the type of nRead to ssize_t (note
the two s), and change the if-condition to
if (static_cast<size_t>(nRead) < buf.size() - 1) {
This is 100% safe: We know that nRead > 0 at the time the cast occurs, and
ssize_t has the same number of bits as size_t, and buf.size() returns
size_t, and buf.size() is never 0. The static_cast is better than a C-style
cast, since you can search for it, and it expresses what is wanted (C-style
casts could also be used to cast constness away etc).

Thanks for the explanation, Georg. I've read many times to favor
static_cast over C-style casts but it hasn't sunk in yet since I
haven't written it myself enough.

The patch is in at fc19148f. Thanks Richard, Cyrille, and Georg for
the discussion.

Scott