Compiler warning after workers/39825

Compiler warning after workers/39825

[Bart Schaefer]

../../zsh-5.0/Src/utils.c: In function `mb_metastrlenend':
../../zsh-5.0/Src/utils.c:5344: warning: comparison is always true due to limited range of data type

commit 9c68ef08

Re: Compiler warning after workers/39825

[Sebastian Gniazdowski]

On Mon, Nov 7, 2016, at 03:50 PM, Bart Schaefer wrote:
> ../../zsh-5.0/Src/utils.c: In function `mb_metastrlenend':
> ../../zsh-5.0/Src/utils.c:5344: warning: comparison is always true due to
> limited range of data type
> 
> commit 9c68ef08

Probably an enlightenment is scheduled on this, but first
rejecting-facts reaction is that you might have modified the line maybe?
Why would char be always in 0 .. 0x7f is a mystery.  Clang doesn't yield
the warning.

BTW. this can be optimized more: `!num_in_char` instead of `complete`,
and no `num_in_char = 0` assignment.

-- 
  Sebastian Gniazdowski
  psprint@fastmail.com

Re: Compiler warning after workers/39825

[Sebastian Gniazdowski]

On Mon, Nov 7, 2016, at 11:08 PM, Sebastian Gniazdowski wrote:
> On Mon, Nov 7, 2016, at 03:50 PM, Bart Schaefer wrote:
> > ../../zsh-5.0/Src/utils.c: In function `mb_metastrlenend':
> > ../../zsh-5.0/Src/utils.c:5344: warning: comparison is always true due to
> > limited range of data type
> > 
> > commit 9c68ef08
> 
> Probably an enlightenment is scheduled on this, but first

Ah, the (mild) enlightenment is: char can be signed / unsigned. Wonder
if this can have any actual implications as 7f is AFAIK still in
unsigned range. For sure there are some techniques of solving such
portability problems hm

-- 
  Sebastian Gniazdowski
  psprint@fastmail.com

Re: Compiler warning after workers/39825

[Peter Stephenson]

On Mon, 07 Nov 2016 23:24:23 -0800
Sebastian Gniazdowski <psprint@fastmail.com> wrote:
> Ah, the (mild) enlightenment is: char can be signed / unsigned. Wonder
> if this can have any actual implications as 7f is AFAIK still in
> unsigned range. For sure there are some techniques of solving such
> portability problems hm

Yes, that's it.  We have a macro to perform a safe cast to unsigned.
That's the right answer, since if the char is signed and is negative,
it's *not* in a range we want to deal with.  So the compiler was right...

Casting 0x74 is unnecessary, since the constant will conform with
unsigned char anyway, but I've made the point...

pws

diff --git a/Src/utils.c b/Src/utils.c
index 733f570..d73110a 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -5341,7 +5341,7 @@ mb_metastrlenend(char *ptr, int width, char *eptr)
 	    inchar = *ptr;
 	ptr++;
 
-	if (complete && (inchar >= 0 && inchar <= 0x7f)) {
+	if (complete && (inchar >= 0 && STOUC(inchar) <= STOUC(0x7f))) {
 	    /*
 	     * We rely on 7-bit US-ASCII as a subset, so skip
 	     * multibyte handling if we have such a character.

Re: Compiler warning after workers/39825

[Peter Stephenson]

On Tue, 8 Nov 2016 10:01:49 +0000
Peter Stephenson <p.stephenson@samsung.com> wrote:
> diff --git a/Src/utils.c b/Src/utils.c
> index 733f570..d73110a 100644
> --- a/Src/utils.c
> +++ b/Src/utils.c
> @@ -5341,7 +5341,7 @@ mb_metastrlenend(char *ptr, int width, char *eptr)
>  	    inchar = *ptr;
>  	ptr++;
>  
> -	if (complete && (inchar >= 0 && inchar <= 0x7f)) {
> +	if (complete && (inchar >= 0 && STOUC(inchar) <= STOUC(0x7f))) {
                         ^^^^^^^^^^^^

This is now irrelevant, I'll remove that, too.

pws

Re: Compiler warning after workers/39825

[Daniel Shahaf]

Peter Stephenson wrote on Tue, Nov 08, 2016 at 10:07:35 +0000:
> On Tue, 8 Nov 2016 10:01:49 +0000
> Peter Stephenson <p.stephenson@samsung.com> wrote:
> > diff --git a/Src/utils.c b/Src/utils.c
> > index 733f570..d73110a 100644
> > --- a/Src/utils.c
> > +++ b/Src/utils.c
> > @@ -5341,7 +5341,7 @@ mb_metastrlenend(char *ptr, int width, char *eptr)
> >  	    inchar = *ptr;
> >  	ptr++;
> >  
> > -	if (complete && (inchar >= 0 && inchar <= 0x7f)) {
> > +	if (complete && (inchar >= 0 && STOUC(inchar) <= STOUC(0x7f))) {
>                          ^^^^^^^^^^^^
> 
> This is now irrelevant, I'll remove that, too.

It's relevant on systems where the «char» type is signed.

Re: Compiler warning after workers/39825

[Peter Stephenson]

On Tue, 8 Nov 2016 15:38:30 +0000
Daniel Shahaf <d.s@daniel.shahaf.name> wrote:

> Peter Stephenson wrote on Tue, Nov 08, 2016 at 10:07:35 +0000:
> > On Tue, 8 Nov 2016 10:01:49 +0000
> > Peter Stephenson <p.stephenson@samsung.com> wrote:
> > > diff --git a/Src/utils.c b/Src/utils.c
> > > index 733f570..d73110a 100644
> > > --- a/Src/utils.c
> > > +++ b/Src/utils.c
> > > @@ -5341,7 +5341,7 @@ mb_metastrlenend(char *ptr, int width, char *eptr)
> > >  	    inchar = *ptr;
> > >  	ptr++;
> > >  
> > > -	if (complete && (inchar >= 0 && inchar <= 0x7f)) {
> > > +	if (complete && (inchar >= 0 && STOUC(inchar) <= STOUC(0x7f))) {
> >                          ^^^^^^^^^^^^
> > 
> > This is now irrelevant, I'll remove that, too.
> 
> It's relevant on systems where the «char» type is signed.

No, read what the remaining modified test does.  It's not using a signed
value any more.

pws

Re: Compiler warning after workers/39825

[Sebastian Gniazdowski]

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]

On Mon, Nov 7, 2016, at 03:50 PM, Bart Schaefer wrote:
> ../../zsh-5.0/Src/utils.c: In function `mb_metastrlenend':
> ../../zsh-5.0/Src/utils.c:5344: warning: comparison is always true due to
> limited range of data type
> 
> commit 9c68ef08

Attached is patch that should work for both signed and unsigned
integers. Does it emit any warning?

On current system warmup the test results are 2141 ms vs 2337 ms,
slightly less than 10%, but still ~200 ms difference.

-- 
  Sebastian Gniazdowski
  psprint@fastmail.com

[-- Attachment #2: ascii_fix.diff --]
[-- Type: text/plain, Size: 629 bytes --]

diff --git a/Src/utils.c b/Src/utils.c
index 733f570..32e7dc6 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -5341,7 +5341,12 @@ mb_metastrlenend(char *ptr, int width, char *eptr)
 	    inchar = *ptr;
 	ptr++;
 
-	if (complete && (inchar >= 0 && inchar <= 0x7f)) {
+        /*
+         * 0x7f is all ones except for top bit in binary,
+         * this will check if inchar is in range 0 .. 127
+         * inclusive, regardless of char signedness
+         */
+	if (complete && (inchar & 0x7f) == inchar) {
 	    /*
 	     * We rely on 7-bit US-ASCII as a subset, so skip
 	     * multibyte handling if we have such a character.