[PATCH] avoid -Wsign-compare warnings

[Pádraig Brady]

I noticed Jim fixed a couple obvious
issues highlighted by the -Wsigned-compare gcc option
we were talking about a few days ago.

The attached patch silences all other instances,
so that this option may be used to help find these
hard to spot errors, when they're introduced in future.

Note this patch is just for comment at this stage.
Also note it is not tested on 64 bit.

thanks,
Pádraig.

>From e22c8a33c74d0d5224f5b542bc366a689c3b4231 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?P=C3=A1draig=20Brady?= <address@hidden>
Date: Thu, 3 Jul 2008 19:27:18 +0100
Subject: [PATCH] avoid -Wsign-compare warnings

* src/copy.c: Cast st_size to umaxint_t in unsigned comparisons,
Use already assigned unsigned variable n, rather than signed n_read
in unsigned comparison.
* src/dd.c: Cast pointer arithmetic to unsigned, and add an assert,
to ensure this is valid. Store lseek return in uintmax_t as it's
used in various unsigned comparisions. Cast to off_t only in error check.
* src/du.c: Store st_size in uintmax_t explicitly to avoid warning.
Also add an assert, to check for the improbable overflow condition.
* src/group-list.c: Use int rather than size_t as variable is used
in signed comparisons.
* src/id.c: Likewise.
* src/ls.c: Cast widths to unsigned as always positive.
* src/od.c: Use unsigned widths as always positive.
* src/pathchk.c: Compare pathconf limits to signed MAX constants,
as pathconf returns signed values.
* src/pr.c: Use unsigned variables in unsigned comparisons.
* src/shuf.c: Likewise.
* src/ptx.c: Cast st_size to umaxint_t in unsigned comparison.
* src/shred.c: Use already assigned signed variable sizeof_r,
rather than the unsigned sizeof(r). Don't use signed integer
overflow check that comtemporary compilers may remove.
* src/sort.c: Use unsigned file_size in unsigned comparisions,
which is valid as we're now checking st.st_size >= 0.
* src/system.h: Cast st_blksize to uintmax_t as it's being compared
to an unsigned MAX constant.
* src/tac.c: Store lseek return in uintmax_t as it's used in
various unsigned comparisions. Cast to off_t only in error check.
---
 src/copy.c       |    7 ++++---
 src/dd.c         |   10 ++++++----
 src/du.c         |   11 ++++++++---
 src/group-list.c |    2 +-
 src/id.c         |    2 +-
 src/ls.c         |    7 ++++---
 src/od.c         |    4 ++--
 src/pathchk.c    |    4 ++--
 src/pr.c         |    6 +++---
 src/ptx.c        |    4 ++--
 src/shred.c      |    4 ++--
 src/shuf.c       |    2 +-
 src/sort.c       |    4 ++--
 src/system.h     |    2 +-
 src/tac.c        |    4 ++--
 15 files changed, 41 insertions(+), 32 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index 82c6978..54a87ca 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -528,7 +528,8 @@ copy_reg (char const *src_name, char const *dst_name,
 
        /* Do not bother with a buffer larger than the input file, plus one
           byte to make sure the file has not grown while reading it.  */
-       if (S_ISREG (src_open_sb.st_mode) && src_open_sb.st_size < buf_size)
+       if (S_ISREG (src_open_sb.st_mode) &&
+           (uintmax_t) src_open_sb.st_size < buf_size)
          buf_size = src_open_sb.st_size + 1;
 
        /* However, stick with a block size that is a positive multiple of
@@ -621,7 +622,7 @@ copy_reg (char const *src_name, char const *dst_name,
            last_write_made_hole = false;
 
            /* A short read on a regular file means EOF.  */
-           if (n_read != buf_size && S_ISREG (src_open_sb.st_mode))
+           if (n != buf_size && S_ISREG (src_open_sb.st_mode))
              break;
          }
       }
@@ -1867,7 +1868,7 @@ copy_internal (char const *src_name, char const *dst_name,
          int saved_errno = errno;
          bool same_link = false;
          if (x->update && !new_dst && S_ISLNK (dst_sb.st_mode)
-             && dst_sb.st_size == strlen (src_link_val))
+             && (uintmax_t) dst_sb.st_size == strlen (src_link_val))
            {
              /* See if the destination is already the desired symlink.
                 FIXME: This behavior isn't documented, and seems wrong
diff --git a/src/dd.c b/src/dd.c
index ead9574..64b055e 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -23,6 +23,7 @@
 #include <sys/types.h>
 #include <signal.h>
 #include <getopt.h>
+#include <assert.h>
 
 #include "system.h"
 #include "error.h"
@@ -848,7 +849,8 @@ parse_symbols (char const *str, struct symbol_value const 
*table,
        {
          if (! entry->symbol[0])
            {
-             size_t slen = strcomma ? strcomma - str : strlen (str);
+             assert (strcomma >= str);
+             size_t slen = strcomma ? (size_t) (strcomma - str) : strlen (str);
              error (0, 0, "%s: %s", _(error_msgid),
                     quotearg_n_style_mem (0, locale_quoting_style, str, slen));
              usage (EXIT_FAILURE);
@@ -1232,7 +1234,7 @@ skip (int fdesc, char const *file, uintmax_t records, 
size_t blocksize,
    be seekable.  */
 
 static bool
-advance_input_after_read_error (size_t nbytes)
+advance_input_after_read_error (off_t nbytes)
 {
   if (! input_seekable)
     {
@@ -1242,7 +1244,7 @@ advance_input_after_read_error (size_t nbytes)
     }
   else
     {
-      off_t offset;
+      uintmax_t offset;
       advance_input_offset (nbytes);
       input_offset_overflow |= (OFF_T_MAX < input_offset);
       if (input_offset_overflow)
@@ -1252,7 +1254,7 @@ advance_input_after_read_error (size_t nbytes)
          return false;
        }
       offset = lseek (STDIN_FILENO, 0, SEEK_CUR);
-      if (0 <= offset)
+      if (0 <= (off_t) offset)
        {
          off_t diff;
          if (offset == input_offset)
diff --git a/src/du.c b/src/du.c
index ee24269..34e96b0 100644
--- a/src/du.c
+++ b/src/du.c
@@ -524,10 +524,15 @@ process_file (FTS *fts, FTSENT *ent)
     }
   else
     {
+      uintmax_t size = sb->st_size;
+      if (!apparent_size)
+       {
+         uintmax_t blocks = ST_NBLOCKS (*sb);
+         assert (blocks <= UINTMAX_MAX/ST_NBLOCKSIZE);
+         size = blocks * ST_NBLOCKSIZE;
+       }
       duinfo_set (&dui,
-                 (apparent_size
-                  ? sb->st_size
-                  : (uintmax_t) ST_NBLOCKS (*sb) * ST_NBLOCKSIZE),
+                 size,
                  (time_type == time_mtime ? get_stat_mtime (sb)
                   : time_type == time_atime ? get_stat_atime (sb)
                   : get_stat_ctime (sb)));
diff --git a/src/group-list.c b/src/group-list.c
index 96438d9..633203d 100644
--- a/src/group-list.c
+++ b/src/group-list.c
@@ -57,7 +57,7 @@ print_group_list (const char *username,
 #if HAVE_GETGROUPS
   {
     GETGROUPS_T *groups;
-    size_t i;
+    int i;
 
     int n_groups = mgetgroups (username, (pwd ? pwd->pw_gid : (gid_t) -1),
                                &groups);
diff --git a/src/id.c b/src/id.c
index 14b558d..8594d47 100644
--- a/src/id.c
+++ b/src/id.c
@@ -299,7 +299,7 @@ print_full_info (const char *username)
 #if HAVE_GETGROUPS
   {
     GETGROUPS_T *groups;
-    size_t i;
+    int i;
 
     int n_groups = mgetgroups (username, (pwd ? pwd->pw_gid : (gid_t) -1),
                               &groups);
diff --git a/src/ls.c b/src/ls.c
index 4b69f7d..970d593 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -4012,17 +4012,18 @@ length_of_file_name_and_frills (const struct fileinfo 
*f)
   if (print_inode)
     len += 1 + (format == with_commas
                ? strlen (umaxtostr (f->stat.st_ino, buf))
-               : inode_number_width);
+               : (unsigned int) inode_number_width);
 
   if (print_block_size)
     len += 1 + (format == with_commas
                ? strlen (human_readable (ST_NBLOCKS (f->stat), buf,
                                          human_output_opts, ST_NBLOCKSIZE,
                                          output_block_size))
-               : block_size_width);
+               : (unsigned int) block_size_width);
 
   if (print_scontext)
-    len += 1 + (format == with_commas ? strlen (f->scontext) : scontext_width);
+    len += 1 + (format == with_commas
+               ? strlen (f->scontext) : (unsigned int) scontext_width);
 
   quote_name (NULL, f->name, filename_quoting_options, &name_width);
   len += name_width;
diff --git a/src/od.c b/src/od.c
index 5b4b7bd..b391336 100644
--- a/src/od.c
+++ b/src/od.c
@@ -118,7 +118,7 @@ struct tspec
                            char const *fmt, int width, int pad);
     char fmt_string[FMT_BYTES_ALLOCATED]; /* Of the style "%*d".  */
     bool hexl_mode_trailer;
-    int field_width; /* Minimum width of a field, excluding leading space.  */
+    unsigned int field_width; /* Minimum width, excluding leading space.  */
     int pad_width; /* Total padding to be divided among fields.  */
   };
 
@@ -1882,7 +1882,7 @@ it must be one character from [doxn]"),
   for (i = 0; i < n_specs; i++)
     {
       int fields_per_block = bytes_per_block / width_bytes[spec[i].size];
-      int block_width = (spec[i].field_width + 1) * fields_per_block;
+      unsigned int block_width = (spec[i].field_width + 1) * fields_per_block;
       if (width_per_block < block_width)
        width_per_block = block_width;
     }
diff --git a/src/pathchk.c b/src/pathchk.c
index 48001fc..5bdac9f 100644
--- a/src/pathchk.c
+++ b/src/pathchk.c
@@ -323,7 +323,7 @@ validate_file_name (char *file, bool 
check_basic_portability,
                     dir);
              return false;
            }
-         maxsize = MIN (size, SIZE_MAX);
+         maxsize = MIN (size, SSIZE_MAX);
        }
 
       if (maxsize <= filelen)
@@ -385,7 +385,7 @@ validate_file_name (char *file, bool 
check_basic_portability,
              len = pathconf (dir, _PC_NAME_MAX);
              *start = c;
              if (0 <= len)
-               name_max = MIN (len, SIZE_MAX);
+               name_max = MIN (len, SSIZE_MAX);
              else
                switch (errno)
                  {
diff --git a/src/pr.c b/src/pr.c
index 6ffe8f1..c3da85f 100644
--- a/src/pr.c
+++ b/src/pr.c
@@ -456,7 +456,7 @@ static char *buff;
 
 /* Index of the position in buff where the next character
    will be stored. */
-static int buff_current;
+static unsigned int buff_current;
 
 /* The number of characters in buff.
    Used for allocation of buff and to detect overflow of buff. */
@@ -1945,8 +1945,8 @@ static void
 store_columns (void)
 {
   int i, j;
-  int line = 0;
-  int buff_start;
+  unsigned int line = 0;
+  unsigned int buff_start;
   int last_col;                /* The rightmost column which will be saved in 
buff */
   COLUMN *p;
 
diff --git a/src/ptx.c b/src/ptx.c
index 827d22e..b7385ec 100644
--- a/src/ptx.c
+++ b/src/ptx.c
@@ -544,8 +544,8 @@ swallow_file_in_memory (const char *file_name, BLOCK *block)
       block->start = xmalloc ((size_t) stat_block.st_size);
 
       if ((in_memory_size = read (file_handle,
-                                 block->start, (size_t) stat_block.st_size))
-         != stat_block.st_size)
+                                 block->start, stat_block.st_size))
+         != (uintmax_t) stat_block.st_size)
        {
 #if MSDOS
          /* On MSDOS, in memory size may be smaller than the file
diff --git a/src/shred.c b/src/shred.c
index 9e82cf2..f19bd21 100644
--- a/src/shred.c
+++ b/src/shred.c
@@ -399,7 +399,7 @@ dopass (int fd, char const *qname, off_t *sizep, int type,
   /* Constant fill patterns need only be set up once. */
   if (type >= 0)
     {
-      lim = (0 <= size && size < sizeof_r ? size : sizeof r);
+      lim = (0 <= size && size < sizeof_r ? size : sizeof_r);
       fillpattern (type, r.u, lim);
       passname (r.u, pass_string);
     }
@@ -488,7 +488,7 @@ dopass (int fd, char const *qname, off_t *sizep, int type,
 
       /* Okay, we have written "soff" bytes. */
 
-      if (offset + soff < offset)
+      if (offset > OFF_T_MAX - (off_t) soff)
        {
          error (0, 0, _("%s: file too large"), qname);
          return -1;
diff --git a/src/shuf.c b/src/shuf.c
index ca5345b..3692e87 100644
--- a/src/shuf.c
+++ b/src/shuf.c
@@ -164,7 +164,7 @@ read_input (FILE *in, char eolbyte, char ***pline)
       off_t current_offset = ftello (in);
       if (0 <= current_offset)
        {
-         off_t remaining_size =
+         uintmax_t remaining_size =
            (current_offset < file_size ? file_size - current_offset : 0);
          if (SIZE_MAX - 2 < remaining_size)
            xalloc_die ();
diff --git a/src/sort.c b/src/sort.c
index 9f998a6..e034fd9 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -1252,7 +1252,7 @@ sort_buffer_size (FILE *const *fps, size_t nfps,
   for (i = 0; i < nfiles; i++)
     {
       struct stat st;
-      off_t file_size;
+      uintmax_t file_size;
       size_t worst_case;
 
       if ((i < nfps ? fstat (fileno (fps[i]), &st)
@@ -1261,7 +1261,7 @@ sort_buffer_size (FILE *const *fps, size_t nfps,
          != 0)
        die (_("stat failed"), files[i]);
 
-      if (S_ISREG (st.st_mode))
+      if (S_ISREG (st.st_mode) && st.st_size >= 0)
        file_size = st.st_size;
       else
        {
diff --git a/src/system.h b/src/system.h
index 4b8e58e..b0f9430 100644
--- a/src/system.h
+++ b/src/system.h
@@ -197,7 +197,7 @@ enum
    anyone knows of a system for which this limit is too small, please
    report it as a bug in this code.  */
 # define ST_BLKSIZE(statbuf) ((0 < (statbuf).st_blksize \
-                              && (statbuf).st_blksize <= SIZE_MAX / 8 + 1) \
+                              && (uintmax_t) (statbuf).st_blksize <= SIZE_MAX 
/ 8 + 1) \
                              ? (statbuf).st_blksize : DEV_BSIZE)
 # if defined hpux || defined __hpux__ || defined __hpux
 /* HP-UX counts st_blocks in 1024-byte units.
diff --git a/src/tac.c b/src/tac.c
index 9cf6d60..17fbb31 100644
--- a/src/tac.c
+++ b/src/tac.c
@@ -204,7 +204,7 @@ tac_seekable (int input_fd, const char *file)
   size_t saved_record_size;
 
   /* Offset in the file of the next read. */
-  off_t file_pos;
+  uintmax_t file_pos;
 
   /* True if `output' has not been called yet for any file.
      Only used when the separator is attached to the preceding record. */
@@ -215,7 +215,7 @@ tac_seekable (int input_fd, const char *file)
 
   /* Find the size of the input file. */
   file_pos = lseek (input_fd, (off_t) 0, SEEK_END);
-  if (file_pos < 1)
+  if ((off_t) file_pos < 1)
     return true;                       /* It's an empty file. */
 
   /* Arrange for the first read to lop off enough to leave the rest of the
-- 
1.5.3.6