Re: [Spice-devel] [PATCH 2/2] Eliminate signed/unsigned warning

[Christophe Fergeau]

On Mon, Feb 19, 2018 at 03:06:40PM +0100, Christophe de Dinechin wrote:
> 
> 
> > On 19 Feb 2018, at 13:24, Christophe Fergeau  wrote:
> > 
> > On Fri, Feb 16, 2018 at 04:23:06PM +0100, Christophe de Dinechin wrote:
> >> From: Christophe de Dinechin 
> >> 
> >> 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


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 2/2] Eliminate signed/unsigned warning

[Christophe de Dinechin]

> On 19 Feb 2018, at 13:24, Christophe Fergeau  wrote:
> 
> On Fri, Feb 16, 2018 at 04:23:06PM +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin 
>> 
>> 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 
>> ---
>> 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 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, , 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

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 2/2] Eliminate signed/unsigned warning

[Christophe Fergeau]

On Fri, Feb 16, 2018 at 04:23:06PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> 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 
> ---
>  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 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, , 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


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 2/2] Eliminate signed/unsigned warning

[Frediano Ziglio]

> 
> From: Christophe de Dinechin 
> 
> 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 
> ---
>  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 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, , 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
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel