Re: argp warnings

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Bruno Haible on 5/8/2009 4:28 PM:
> Eric Blake wrote:
>> *** argp.11760       Fri May  8 08:56:27 2009
>> --- -        Fri May  8 08:56:27 2009
>> ***************
>> *** 1,4 ****
>>   Usage: test-argp [-tvCSOlp?V] [-f FILE] [-o[ARG]] [--test] [--file=FILE]
>>               [--input=FILE] [--verbose] [--cantiga] [--sonet] [--option]
>> !             [--optional[=ARG]] [--limerick] [--poem] [--help] [--usage]
>> !             [--version] ARGS...
>> --- 1,4 ----
>>   Usage: test-argp [-tvCSOlp?V] [-f FILE] [-o[ARG]] [--test] [--file=FILE]
>>               [--input=FILE] [--verbose] [--cantiga] [--sonet] [--option]
>> !             [--optional[=ARG]] [--limerick] [--poem] [--help] [--version]
>> !             [--usage] ARGS...
> 
> This was already reported in 2007 [1] and 2008 [2]. I believe the cause is
> that the qsort() call in lib/argp-help.c is sensitive to the implementation
> of qsort. In other words, the comparison criterion passed to qsort may return > 0
> for different entries.
> 
> Bruno
> 
> [1] http://lists.gnu.org/archive/html/bug-gnulib/2007-03/msg00274.html
> [2] http://lists.gnu.org/archive/html/bug-gnulib/2008-04/msg00213.html

I finally stepped through this in the debugger, and found the real reason
for the test failure.  qsort is being used in a completely stable manner;
however, the comparison between --usage (no short opt) and --version
(short opt V) is violating POSIX, by calling _tolower('u').  On glibc,
this results in 'u'|0x20 == 'u', but on other systems (where the test is
failing), it results in 'u'+0x20 == 0x95.  Thus, the comparison between
_tolower('u') and _tolower('V') gives the wrong result.  And even the
tolower code was not 8-bit clean.

OK to commit this patch?

- --
Don't work too hard, make some time for fun as well!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkq+3YAACgkQ84KuGfSFAYCs5QCgnMarFZ2dxWOHaKKKochNMIaj
nukAoLXdIXMDq+04rHdAAV/ShNJy4/qq
=tLEb
-----END PGP SIGNATURE-----

>From 5e645833514ee85138029a2f858e4c96186d6443 Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Sat, 26 Sep 2009 21:32:14 -0600
Subject: [PATCH] argp: fix test failure

* lib/argp-help.c (hol_entry_cmp): Don't use _tolower on values
that are not upper-case.  Pass correct range to tolower.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog       |    6 ++++++
 lib/argp-help.c |   12 +++++-------
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 6558ac3..71d8d46 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2009-09-27  Eric Blake  <address@hidden>
+
+       argp: fix test failure
+       * lib/argp-help.c (hol_entry_cmp): Don't use _tolower on values
+       that are not upper-case.  Pass correct range to tolower.
+
 2009-09-26  Eric Blake  <address@hidden>

        doc: mention yet more cygwin 1.7 status
diff --git a/lib/argp-help.c b/lib/argp-help.c
index a9843c0..150a0ad 100644
--- a/lib/argp-help.c
+++ b/lib/argp-help.c
@@ -1,5 +1,5 @@
 /* Hierarchial argument parsing help output
-   Copyright (C) 1995-2005, 2007 Free Software Foundation, Inc.
+   Copyright (C) 1995-2005, 2007, 2009 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Written by Miles Bader <address@hidden>.

@@ -797,13 +797,11 @@ hol_entry_cmp (const struct hol_entry *entry1,
           first, but as they're not displayed, it doesn't matter where
           they are.  */
        {
-         char first1 = short1 ? short1 : long1 ? *long1 : 0;
-         char first2 = short2 ? short2 : long2 ? *long2 : 0;
-#ifdef _tolower
-         int lower_cmp = _tolower (first1) - _tolower (first2);
-#else
+         unsigned char first1 = short1 ? short1 : long1 ? *long1 : 0;
+         unsigned char first2 = short2 ? short2 : long2 ? *long2 : 0;
+         /* Use tolower, not _tolower, since only the former is
+            guaranteed to work on something already lower case.  */
          int lower_cmp = tolower (first1) - tolower (first2);
-#endif
          /* Compare ignoring case, except when the options are both the
             same letter, in which case lower-case always comes first.  */
          return lower_cmp ? lower_cmp :
-- 
1.6.5.rc1