Re: [Spice-devel] [PATCH 2/2] Eliminate signed/unsigned warning
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
> 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, &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 ___ 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
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, &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 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
> > 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, &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 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel