[2/2] Eliminate signed/unsigned warning

Submitted by Christophe de Dinechin on Feb. 16, 2018, 3:23 p.m.

Details

Message ID 20180216152306.25482-3-christophe@dinechin.org
State New
Headers show
Series "Warning and build error fixes" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Christophe de Dinechin Feb. 16, 2018, 3:23 p.m.
From: Christophe de Dinechin <dinechin@redhat.com>

Without this, GCC complains about signed / unsigned comparisons:

mjpeg-fallback.cpp:121:24: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
     if (win_info.width != last_width || win_info.height != last_height) {
         ~~~~~~~~~~~~~~~^~~~~~~~~~~~~

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 src/mjpeg-fallback.cpp        | 2 +-
 src/spice-streaming-agent.cpp | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
index 634864f..53804d9 100644
--- a/src/mjpeg-fallback.cpp
+++ b/src/mjpeg-fallback.cpp
@@ -57,7 +57,7 @@  private:
     std::vector<uint8_t> frame;
 
     // last frame sizes
-    uint32_t last_width = ~0u, last_height = ~0u;
+    int last_width = ~0u, last_height = ~0u;
     // last time before capture
     uint64_t last_time = 0;
 };
diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 4ec5e42..27b26a4 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -106,7 +106,7 @@  static int read_command_from_device(void)
         return 0; // return -1;
     }
     n = read(streamfd, &msg, hdr.size);
-    if (n != hdr.size) {
+    if (n != (int) hdr.size) {
         syslog(LOG_WARNING,
                "read command from device FAILED -- read %d expected %d\n",
                n, hdr.size);

Comments

> 
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> Without this, GCC complains about signed / unsigned comparisons:
> 
> mjpeg-fallback.cpp:121:24: warning: comparison between signed and unsigned
> integer expressions [-Wsign-compare]
>      if (win_info.width != last_width || win_info.height != last_height) {
>          ~~~~~~~~~~~~~~~^~~~~~~~~~~~~
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  src/mjpeg-fallback.cpp        | 2 +-
>  src/spice-streaming-agent.cpp | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> index 634864f..53804d9 100644
> --- a/src/mjpeg-fallback.cpp
> +++ b/src/mjpeg-fallback.cpp
> @@ -57,7 +57,7 @@ private:
>      std::vector<uint8_t> frame;
>  
>      // last frame sizes
> -    uint32_t last_width = ~0u, last_height = ~0u;
> +    int last_width = ~0u, last_height = ~0u;
>      // last time before capture
>      uint64_t last_time = 0;
>  };

That is weird, so the compiler here is not giving any warning
doing an implicit unsigned -> signed conversion which should
be explicit!
Not that the ranges here is a problem.
On the other way a window with a negative width is very hard to
picture, we are basically hiding what the compiler see as a possible
issue, is not much different that just disabling the warning at
the end.

> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 4ec5e42..27b26a4 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -106,7 +106,7 @@ static int read_command_from_device(void)
>          return 0; // return -1;
>      }
>      n = read(streamfd, &msg, hdr.size);
> -    if (n != hdr.size) {
> +    if (n != (int) hdr.size) {
>          syslog(LOG_WARNING,
>                 "read command from device FAILED -- read %d expected %d\n",
>                 n, hdr.size);

Acked

Frediano
On Fri, Feb 16, 2018 at 04:23:06PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> Without this, GCC complains about signed / unsigned comparisons:
> 
> mjpeg-fallback.cpp:121:24: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
>      if (win_info.width != last_width || win_info.height != last_height) {
>          ~~~~~~~~~~~~~~~^~~~~~~~~~~~~

Are you getting this using the default CXXFLAGS? Here I seem to be getting
-Wno-sign-compare by default.

Christophe

> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  src/mjpeg-fallback.cpp        | 2 +-
>  src/spice-streaming-agent.cpp | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> index 634864f..53804d9 100644
> --- a/src/mjpeg-fallback.cpp
> +++ b/src/mjpeg-fallback.cpp
> @@ -57,7 +57,7 @@ private:
>      std::vector<uint8_t> frame;
>  
>      // last frame sizes
> -    uint32_t last_width = ~0u, last_height = ~0u;
> +    int last_width = ~0u, last_height = ~0u;
>      // last time before capture
>      uint64_t last_time = 0;
>  };
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 4ec5e42..27b26a4 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -106,7 +106,7 @@ static int read_command_from_device(void)
>          return 0; // return -1;
>      }
>      n = read(streamfd, &msg, hdr.size);
> -    if (n != hdr.size) {
> +    if (n != (int) hdr.size) {
>          syslog(LOG_WARNING,
>                 "read command from device FAILED -- read %d expected %d\n",
>                 n, hdr.size);
> -- 
> 2.13.5 (Apple Git-94)
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> On 19 Feb 2018, at 13:24, Christophe Fergeau <cfergeau@redhat.com> wrote:
> 
> On Fri, Feb 16, 2018 at 04:23:06PM +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinechin@redhat.com>
>> 
>> Without this, GCC complains about signed / unsigned comparisons:
>> 
>> mjpeg-fallback.cpp:121:24: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
>>     if (win_info.width != last_width || win_info.height != last_height) {
>>         ~~~~~~~~~~~~~~~^~~~~~~~~~~~~
> 
> Are you getting this using the default CXXFLAGS?

No, this was a side effect of tracking some other warning.


> Here I seem to be getting
> -Wno-sign-compare by default.

Why is this silenced? There aren’t that many of them, and implicit int promotion makes some scenarios risky, e.g signed byte vs unsigned byte.

Hmm, I checked, and the “signed-compare” of GCC does not warn in the dangerous cases, only in the harmless ones… Weird.

No warning for this, where the result is “surprisingly” false because of int promotion (comparison is false), even with -Wall, -Wextra or -Wpedantic
    signed char i = -3;
    unsigned char j = -3;
    printf("i=%x j=%x i==j=%d\n", i, j, i==j);

But a warning for this where the result is true as expected:
    signed char i = -3;
    unsigned char j = -3;
    printf("i=%x j=%x i==j=%d\n", i, j, i==j);

Really strange. Well, if that’s how the option behaves, then -Wno-sign-compare seems harmless.

But does anybody understand the rationale for this? I’m a bit puzzled.

> 
> Christophe
> 
>> 
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>> src/mjpeg-fallback.cpp        | 2 +-
>> src/spice-streaming-agent.cpp | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
>> index 634864f..53804d9 100644
>> --- a/src/mjpeg-fallback.cpp
>> +++ b/src/mjpeg-fallback.cpp
>> @@ -57,7 +57,7 @@ private:
>>     std::vector<uint8_t> frame;
>> 
>>     // last frame sizes
>> -    uint32_t last_width = ~0u, last_height = ~0u;
>> +    int last_width = ~0u, last_height = ~0u;
>>     // last time before capture
>>     uint64_t last_time = 0;
>> };
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index 4ec5e42..27b26a4 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -106,7 +106,7 @@ static int read_command_from_device(void)
>>         return 0; // return -1;
>>     }
>>     n = read(streamfd, &msg, hdr.size);
>> -    if (n != hdr.size) {
>> +    if (n != (int) hdr.size) {
>>         syslog(LOG_WARNING,
>>                "read command from device FAILED -- read %d expected %d\n",
>>                n, hdr.size);
>> -- 
>> 2.13.5 (Apple Git-94)
>> 
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Mon, Feb 19, 2018 at 03:06:40PM +0100, Christophe de Dinechin wrote:
> 
> 
> > On 19 Feb 2018, at 13:24, Christophe Fergeau <cfergeau@redhat.com> wrote:
> > 
> > On Fri, Feb 16, 2018 at 04:23:06PM +0100, Christophe de Dinechin wrote:
> >> From: Christophe de Dinechin <dinechin@redhat.com>
> >> 
> >> Without this, GCC complains about signed / unsigned comparisons:
> >> 
> >> mjpeg-fallback.cpp:121:24: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
> >>     if (win_info.width != last_width || win_info.height != last_height) {
> >>         ~~~~~~~~~~~~~~~^~~~~~~~~~~~~
> > 
> > Are you getting this using the default CXXFLAGS?
> 
> No, this was a side effect of tracking some other warning.

Would be nice to mention this in the commit log then, while we want the
build with the default flags to be warning-free, fixing additional
warnings will definitely be considered on a case by case basis.

> 
> 
> > Here I seem to be getting -Wno-sign-compare by default.
> 
> Why is this silenced?

This probably was not intentionally silenced, but enabling it would be a
good complement to this patch if this warning is deemed useful...

Christophe