Re: avifil32: Removed sign comparison warning (sizeof expresions)
Marko Nikolic wrote:
sizeof expresion always return unsigned type, so it is casted when comparing with signed values. This is ugly.
--- dlls/avifil32/wavfile.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/dlls/avifil32/wavfile.c b/dlls/avifil32/wavfile.c index fd6d9ec..3fc934c 100644 --- a/dlls/avifil32/wavfile.c +++ b/dlls/avifil32/wavfile.c @@ -803,7 +803,7 @@ static HRESULT WINAPI IAVIStream_fnSetFormat(IAVIStream *iface, LONG pos, TRACE("(%p,%d,%p,%d)\n", iface, pos, format, formatsize);
/* check parameters */ - if (format == NULL || formatsize <= sizeof(PCMWAVEFORMAT)) + if (format == NULL || formatsize <= (LONG)sizeof(PCMWAVEFORMAT)) IMHO gcc is *wrong* in emitting a warning there. sizeof(PCMWAVEFORMAT) is a compile time constant and gcc can see that sizeof(PCMWAVEFORMAT) falls well inside the number range expressible by a LONG. Logically there is no difference between formatsize <= sizeof(PCMWAVEFORMAT) and formatsize <= 16 One gives a bogus warning and the other doesn't.
bye michael
On Monday 23 August 2010 13:16:07 Michael Stefaniuc wrote:
IMHO gcc is *wrong* in emitting a warning there. sizeof(PCMWAVEFORMAT) is a compile time constant and gcc can see that sizeof(PCMWAVEFORMAT) falls well inside the number range expressible by a LONG. Logically there is no difference between formatsize <= sizeof(PCMWAVEFORMAT) and formatsize <= 16 One gives a bogus warning and the other doesn't.
C99 std (para 6.5.3.4.4) states following about sizeof operator: "...its type (an unsigned integer type) is size_t, defined in <stddef.h> (and other headers)." sizeof result is a compile-time constant but, unlike numeric constants, its type must always be size_t so gcc does the correct thing here.
Andrey Turkin wrote:
On Monday 23 August 2010 13:16:07 Michael Stefaniuc wrote:
IMHO gcc is *wrong* in emitting a warning there. sizeof(PCMWAVEFORMAT) is a compile time constant and gcc can see that sizeof(PCMWAVEFORMAT) falls well inside the number range expressible by a LONG. Logically there is no difference between formatsize <= sizeof(PCMWAVEFORMAT) and formatsize <= 16 One gives a bogus warning and the other doesn't.
C99 std (para 6.5.3.4.4) states following about sizeof operator: "...its type (an unsigned integer type) is size_t, defined in <stddef.h> (and other headers)."
sizeof result is a compile-time constant but, unlike numeric constants, its type must always be size_t so gcc does the correct thing here.
The same is with C89, paragraph 3.3.3.4 from standard: The value of the result is implementation-defined, and its type (an unsigned integral type) is size_t defined in the <stddef.h> header. So, the gcc is correct. In the example above, correct would be formatsize <= 16U which gives a warning, as expected. What remains is how to correctly remove warning. In this case (and there are many similar in the code), signed function parameter is comparing with values that are natively unsigned. Changing type of the parameter is not possible, the same if with sizeof operator. One possiblity is to add some temporary variable, but in my opinioin it will just unncesary bloat the code and is worse than casting return value of sizeof. Maybe better solution is to make signed_sizeof macro, which will always return signed values and use it instead of sizeof; I will check if there is possiblity for that.
Marko Nikolic <grkoma(a)gmail.com> writes:
What remains is how to correctly remove warning. In this case (and there are many similar in the code), signed function parameter is comparing with values that are natively unsigned. Changing type of the parameter is not possible, the same if with sizeof operator. One possiblity is to add some temporary variable, but in my opinioin it will just unncesary bloat the code and is worse than casting return value of sizeof.
In general a negative size would be an error too, so casting the sizeof wouldn't do the right thing. -- Alexandre Julliard julliard(a)winehq.org
Alexandre Julliard wrote:
Marko Nikolic <grkoma(a)gmail.com> writes:
What remains is how to correctly remove warning. In this case (and there are many similar in the code), signed function parameter is comparing with values that are natively unsigned. Changing type of the parameter is not possible, the same if with sizeof operator. One possiblity is to add some temporary variable, but in my opinioin it will just unncesary bloat the code and is worse than casting return value of sizeof.
In general a negative size would be an error too, so casting the sizeof wouldn't do the right thing. Hmm ... gcc would still issue the warning even if we know that formatsize cannot be negative aka formatsize >= 0 && formatsize <= sizeof(PCMWAVEFORMAT) would still produce a warning.
bye michael
Marko Nikolic wrote:
Andrey Turkin wrote:
On Monday 23 August 2010 13:16:07 Michael Stefaniuc wrote:
IMHO gcc is *wrong* in emitting a warning there. sizeof(PCMWAVEFORMAT) is a compile time constant and gcc can see that sizeof(PCMWAVEFORMAT) falls well inside the number range expressible by a LONG. Logically there is no difference between formatsize <= sizeof(PCMWAVEFORMAT) and formatsize <= 16 One gives a bogus warning and the other doesn't. C99 std (para 6.5.3.4.4) states following about sizeof operator: "...its type (an unsigned integer type) is size_t, defined in <stddef.h> (and other headers)."
sizeof result is a compile-time constant but, unlike numeric constants, its type must always be size_t so gcc does the correct thing here.
The same is with C89, paragraph 3.3.3.4 from standard:
The value of the result is implementation-defined, and its type (an unsigned integral type) is size_t defined in the <stddef.h> header.
So, the gcc is correct. In the example above, correct would be
formatsize <= 16U
which gives a warning, as expected. Yeah, sorry about that. I ASSumed the C spec would preserve the value and as sizeof(PCMWAVEFORMAT) is a constant expression it would be promoted to a LONG as that would preserve the values of both sides. gcc has no choice in this case then to follow the C standard. So I've read up on it and now I blame the C standard; especially after reading the "Coding guidelines" on page 7 of http://c0x.coding-guidelines.com/6.5.3.4.pdf aka the commentary to the changes proposed to sizeof() in the next C standard iteration. So much for the "principle of the least surprise" ...
What remains is how to correctly remove warning. In this case (and there are many similar in the code), signed function parameter is comparing with values that are natively unsigned. Changing type of the parameter is not possible, the same if with sizeof operator. One possiblity is to add some temporary variable, but in my opinioin it will just unncesary bloat the code and is worse than casting return value of sizeof.
Maybe better solution is to make signed_sizeof macro, which will always return signed values and use it instead of sizeof; I will check if there is possiblity for that.
bye michael
Andrey Turkin wrote:
On Monday 23 August 2010 13:16:07 Michael Stefaniuc wrote:
IMHO gcc is *wrong* in emitting a warning there. sizeof(PCMWAVEFORMAT) is a compile time constant and gcc can see that sizeof(PCMWAVEFORMAT) falls well inside the number range expressible by a LONG. Logically there is no difference between formatsize <= sizeof(PCMWAVEFORMAT) and formatsize <= 16 One gives a bogus warning and the other doesn't.
C99 std (para 6.5.3.4.4) states following about sizeof operator: "...its type (an unsigned integer type) is size_t, defined in <stddef.h> (and other headers)."
sizeof result is a compile-time constant but, unlike numeric constants, its type must always be size_t so gcc does the correct thing here. That's actually not the problem but the "value preservation rule" especially the "unsigned preservation rule". As sizeof(LONG) <= sizeof(size_t) the formatsize will be promoted to an unsigned. Iff formatsize is negative it will produce a big unsigned integer. So I'll retract my critique of gcc (in this case as the compiler cannot know what values formatsize will hold) and blame the C standard instead.
bye michael
participants (4)
-
Alexandre Julliard -
Andrey Turkin -
Marko Nikolic -
Michael Stefaniuc