GNU bug report logs - #7203
[PATCH] sort: fix unportable conversion of unsigned char * -> char *

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 7203 in the body.
You can then email your comments to 7203 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Bug-coreutils <bug-coreutils <at> gnu.org>
Subject: [PATCH] sort: fix unportable conversion of unsigned char * -> char *
Date: Tue, 12 Oct 2010 13:00:03 -0700

This is another portability bug caught by the Sun C compiler.
Without this patch, cc complains:

"sort.c", line 3933: warning: assignment type mismatch:
        pointer to const char "=" pointer to unsigned char


* src/sort.c (fold_toupper): Change this back from char to
unsigned char, fixing a regression introduced in commit
59e2e55d0f154a388adc9bac37d2b45f2ba971f8 dated February 26, as the
C Standard doesn't let you convert from unsigned char * to char *
without a cast, and the (in theory more portable) style here is to
convert char values, not pointer values.
(getmonth): Convert char to unsigned char when needed for
comparison.
---
 src/sort.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/sort.c b/src/sort.c
index 89f7be3..c155eda 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -268,7 +268,7 @@ static bool nonprinting[UCHAR_LIM];
 static bool nondictionary[UCHAR_LIM];
 
 /* Translation table folding lower case to upper.  */
-static unsigned char fold_toupper[UCHAR_LIM];
+static char fold_toupper[UCHAR_LIM];
 
 #define MONTHS_PER_YEAR 12
 
@@ -1952,12 +1952,12 @@ getmonth (char const *month, char **ea)
                 *ea = (char *) m;
               return monthtab[ix].val;
             }
-          if (fold_toupper[to_uchar (*m)] < to_uchar (*n))
+          if (to_uchar (fold_toupper[to_uchar (*m)]) < to_uchar (*n))
             {
               hi = ix;
               break;
             }
-          else if (fold_toupper[to_uchar (*m)] > to_uchar (*n))
+          else if (to_uchar (fold_toupper[to_uchar (*m)]) > to_uchar (*n))
             {
               lo = ix + 1;
               break;
-- 
1.7.2

Message #8 received at 7203 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 7203 <at> debbugs.gnu.org
Subject: Re: bug#7203: [PATCH] sort: fix unportable conversion of unsigned
	char * -> char *
Date: Tue, 12 Oct 2010 23:28:22 +0200

Paul Eggert wrote:
> This is another portability bug caught by the Sun C compiler.
> Without this patch, cc complains:
>
> "sort.c", line 3933: warning: assignment type mismatch:
>         pointer to const char "=" pointer to unsigned char
>
>
> * src/sort.c (fold_toupper): Change this back from char to
> unsigned char, fixing a regression introduced in commit
> 59e2e55d0f154a388adc9bac37d2b45f2ba971f8 dated February 26, as the
> C Standard doesn't let you convert from unsigned char * to char *
> without a cast, and the (in theory more portable) style here is to
> convert char values, not pointer values.
> (getmonth): Convert char to unsigned char when needed for
> comparison.

Calling commit 59e2e55d0f a regression might lead the unwary
to think that it causes sort to malfunction, when in fact,
at worst (afaics), it evokes a warning.

So how about something like s/regression/problem/ ?

This feels like enough of a technicality that I'd prefer to defer it.

> ---
>  src/sort.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/sort.c b/src/sort.c
> index 89f7be3..c155eda 100644
> --- a/src/sort.c
> +++ b/src/sort.c
> @@ -268,7 +268,7 @@ static bool nonprinting[UCHAR_LIM];
>  static bool nondictionary[UCHAR_LIM];
>
>  /* Translation table folding lower case to upper.  */
> -static unsigned char fold_toupper[UCHAR_LIM];
> +static char fold_toupper[UCHAR_LIM];
>
>  #define MONTHS_PER_YEAR 12
>
> @@ -1952,12 +1952,12 @@ getmonth (char const *month, char **ea)
>                  *ea = (char *) m;
>                return monthtab[ix].val;
>              }
> -          if (fold_toupper[to_uchar (*m)] < to_uchar (*n))
> +          if (to_uchar (fold_toupper[to_uchar (*m)]) < to_uchar (*n))
>              {
>                hi = ix;
>                break;
>              }
> -          else if (fold_toupper[to_uchar (*m)] > to_uchar (*n))
> +          else if (to_uchar (fold_toupper[to_uchar (*m)]) > to_uchar (*n))
>              {
>                lo = ix + 1;
>                break;

Message #11 received at 7203 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Jim Meyering <jim <at> meyering.net>
Cc: 7203 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: bug#7203: [PATCH] sort: fix unportable conversion of unsigned
	char	* -> char *
Date: Tue, 12 Oct 2010 22:54:25 +0100

On 12/10/10 22:28, Jim Meyering wrote:
> Paul Eggert wrote:
>> This is another portability bug caught by the Sun C compiler.
>> Without this patch, cc complains:
>>
>> "sort.c", line 3933: warning: assignment type mismatch:
>>         pointer to const char "=" pointer to unsigned char
>>
>>
>> * src/sort.c (fold_toupper): Change this back from char to
>> unsigned char, fixing a regression introduced in commit
>> 59e2e55d0f154a388adc9bac37d2b45f2ba971f8 dated February 26, as the
>> C Standard doesn't let you convert from unsigned char * to char *
>> without a cast, and the (in theory more portable) style here is to
>> convert char values, not pointer values.
>> (getmonth): Convert char to unsigned char when needed for
>> comparison.
> 
> Calling commit 59e2e55d0f a regression might lead the unwary
> to think that it causes sort to malfunction, when in fact,
> at worst (afaics), it evokes a warning.
> 
> So how about something like s/regression/problem/ ?
> 
> This feels like enough of a technicality that I'd prefer to defer it.

Note gcc would also warn about this with -Wall, except that
we disable the warning with -Wno-pointer-sign in configure.ac

So I agree with the change, but s/regression/warning/

cheers,
Pádraig.

Message #14 received at 7203 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Jim Meyering <jim <at> meyering.net>
Cc: 7203 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: bug#7203: [PATCH] sort: fix unportable conversion of unsigned
	char	* -> char *
Date: Tue, 12 Oct 2010 23:17:45 +0100

On 12/10/10 22:54, Pádraig Brady wrote:
> On 12/10/10 22:28, Jim Meyering wrote:
>> Paul Eggert wrote:
>>> This is another portability bug caught by the Sun C compiler.
>>> Without this patch, cc complains:
>>>
>>> "sort.c", line 3933: warning: assignment type mismatch:
>>>         pointer to const char "=" pointer to unsigned char
>>>
>>>
>>> * src/sort.c (fold_toupper): Change this back from char to
>>> unsigned char, fixing a regression introduced in commit
>>> 59e2e55d0f154a388adc9bac37d2b45f2ba971f8 dated February 26, as the
>>> C Standard doesn't let you convert from unsigned char * to char *
>>> without a cast, and the (in theory more portable) style here is to
>>> convert char values, not pointer values.
>>> (getmonth): Convert char to unsigned char when needed for
>>> comparison.
>>
>> Calling commit 59e2e55d0f a regression might lead the unwary
>> to think that it causes sort to malfunction, when in fact,
>> at worst (afaics), it evokes a warning.
>>
>> So how about something like s/regression/problem/ ?
>>
>> This feels like enough of a technicality that I'd prefer to defer it.
> 
> Note gcc would also warn about this with -Wall, except that
> we disable the warning with -Wno-pointer-sign in configure.ac
> 
> So I agree with the change, but s/regression/warning/

I just compiled coreutils/{src,lib} with make CFLAGS="-Wpointer-sign -Werror"
and the above was the only issue. How about enabling the warning?

commit e6d6fd6ecfce79b8c3f1e4ce7da67b26adbfbb82
Author: Pádraig Brady <P <at> draigBrady.com>
Date:   Tue Oct 12 23:15:23 2010 +0100

    maint: enable the -Wpointer-sign gcc warning

    * configure.ac: -Wall implicit enables this warning
    so remove the explicit disabling.

diff --git a/configure.ac b/configure.ac
index acd397e..7ebf98c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -98,7 +98,6 @@ if test "$gl_gcc_warnings" = yes; then
   done
   gl_WARN_ADD([-Wno-missing-field-initializers]) # We need this one
   gl_WARN_ADD([-Wno-sign-compare])     # Too many warnings for now
-  gl_WARN_ADD([-Wno-pointer-sign])     # Too many warnings for now
   gl_WARN_ADD([-Wno-unused-parameter]) # Too many warnings for now

   # In spite of excluding -Wlogical-op above, it is enabled, as of

Message #17 received at 7203 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Jim Meyering <jim <at> meyering.net>
Cc: 7203 <at> debbugs.gnu.org
Subject: Re: bug#7203: [PATCH] sort: fix unportable conversion of unsigned
	char * -> char *
Date: Tue, 12 Oct 2010 16:23:19 -0700

On 10/12/10 14:28, Jim Meyering wrote:
> This feels like enough of a technicality that I'd prefer to defer it.

Well, my suspicion is that some C compilers simply refuse to compile
the construct, as they're entitled to do.  I vaguely recall that
happening in the past.  No big deal of course.

You're right about the word "regression": I should have called it a "warning"
or "compile-time issue" or something like that, since it's not a run-time
problem on all practical porting hosts.

Message #20 received at 7203 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 7203 <at> debbugs.gnu.org
Subject: Re: bug#7203: [PATCH] sort: fix unportable conversion of unsigned
	char * -> char *
Date: Wed, 13 Oct 2010 06:26:56 +0200

Paul Eggert wrote:
> On 10/12/10 14:28, Jim Meyering wrote:
>> This feels like enough of a technicality that I'd prefer to defer it.
>
> Well, my suspicion is that some C compilers simply refuse to compile
> the construct, as they're entitled to do.  I vaguely recall that
> happening in the past.  No big deal of course.

Could well be.
If fixing this decreases by one the number of bug reports,
then it'll have been worth applying, and I can think of at
least one test-system-menagerie where it may well affect several.
So go ahead and push that after all.

> You're right about the word "regression": I should have called it a "warning"
> or "compile-time issue" or something like that, since it's not a run-time
> problem on all practical porting hosts.

Message #23 received at 7203 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Jim Meyering <jim <at> meyering.net>
Cc: 7203 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: bug#7203: [PATCH] sort: fix unportable conversion of unsigned
	char	* -> char *
Date: Tue, 16 Nov 2010 00:46:25 +0000

On 12/10/10 23:17, Pádraig Brady wrote:
> On 12/10/10 22:54, Pádraig Brady wrote:
>> On 12/10/10 22:28, Jim Meyering wrote:
>>> Paul Eggert wrote:
>>>> This is another portability bug caught by the Sun C compiler.
>>>> Without this patch, cc complains:
>>>>
>>>> "sort.c", line 3933: warning: assignment type mismatch:
>>>>         pointer to const char "=" pointer to unsigned char
>>>>
>>>>
>>>> * src/sort.c (fold_toupper): Change this back from char to
>>>> unsigned char, fixing a regression introduced in commit
>>>> 59e2e55d0f154a388adc9bac37d2b45f2ba971f8 dated February 26, as the
>>>> C Standard doesn't let you convert from unsigned char * to char *
>>>> without a cast, and the (in theory more portable) style here is to
>>>> convert char values, not pointer values.
>>>> (getmonth): Convert char to unsigned char when needed for
>>>> comparison.
>>>
>>> Calling commit 59e2e55d0f a regression might lead the unwary
>>> to think that it causes sort to malfunction, when in fact,
>>> at worst (afaics), it evokes a warning.
>>>
>>> So how about something like s/regression/problem/ ?
>>>
>>> This feels like enough of a technicality that I'd prefer to defer it.
>>
>> Note gcc would also warn about this with -Wall, except that
>> we disable the warning with -Wno-pointer-sign in configure.ac
>>
>> So I agree with the change, but s/regression/warning/
> 
> I just compiled coreutils/{src,lib} with make CFLAGS="-Wpointer-sign -Werror"
> and the above was the only issue. How about enabling the warning?
> 
> commit e6d6fd6ecfce79b8c3f1e4ce7da67b26adbfbb82
> Author: Pádraig Brady <P <at> draigBrady.com>
> Date:   Tue Oct 12 23:15:23 2010 +0100
> 
>     maint: enable the -Wpointer-sign gcc warning
> 
>     * configure.ac: -Wall implicit enables this warning
>     so remove the explicit disabling.
> 
> diff --git a/configure.ac b/configure.ac
> index acd397e..7ebf98c 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -98,7 +98,6 @@ if test "$gl_gcc_warnings" = yes; then
>    done
>    gl_WARN_ADD([-Wno-missing-field-initializers]) # We need this one
>    gl_WARN_ADD([-Wno-sign-compare])     # Too many warnings for now
> -  gl_WARN_ADD([-Wno-pointer-sign])     # Too many warnings for now
>    gl_WARN_ADD([-Wno-unused-parameter]) # Too many warnings for now
> 
>    # In spite of excluding -Wlogical-op above, it is enabled, as of
> 
> 
> 
> 
I just applied that and this follow up

commit 0d1ba34494e5e5e21e205f2e86349afeb69ca84d
Author: Pádraig Brady <P <at> draigBrady.com>
Date:   Tue Nov 16 00:31:05 2010 +0000

    maint: fix a new -Wpointer-sign gcc warning

    * src/csplit.c (max_out): Fix a new warning introduced with
    commit 6568b173, 2010-11-10, "csplit: do not rely on..."

diff --git a/src/csplit.c b/src/csplit.c
index 531e492..07c5c8c 100644
--- a/src/csplit.c
+++ b/src/csplit.c
@@ -1275,7 +1275,7 @@ max_out (char *format)
           error (EXIT_FAILURE, 0,
                  _("too many %% conversion specifications in suffix"));
         percent = true;
-        unsigned int flags;
+        int flags;
         f += get_format_flags (f, &flags);
         while (ISDIGIT (*f))
           f++;

Message #28 received at 7203-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 7203-done <at> debbugs.gnu.org
Subject: Re: bug#7203: [PATCH] sort: fix unportable conversion of unsigned
	char * -> char *
Date: Tue, 19 Apr 2011 09:08:28 +0200

Paul Eggert wrote:
> This is another portability bug caught by the Sun C compiler.
> Without this patch, cc complains:
>
> "sort.c", line 3933: warning: assignment type mismatch:
>         pointer to const char "=" pointer to unsigned char
>
>
> * src/sort.c (fold_toupper): Change this back from char to
> unsigned char, fixing a regression introduced in commit
> 59e2e55d0f154a388adc9bac37d2b45f2ba971f8 dated February 26, as the
> C Standard doesn't let you convert from unsigned char * to char *
> without a cast, and the (in theory more portable) style here is to
> convert char values, not pointer values.
> (getmonth): Convert char to unsigned char when needed for
> comparison.

Thanks again.
The patch went in months ago, so I'm closing this.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.