Re: Small patch for fmt(1)

2013-11-19 Thread drosih 

On Mon, 18 Nov 2013 22:16:54 EST Eitan Adler wrote:

 Hi all,
 
 clang gives a warning when building fmt(1):
 
 fmt.c:400:35: warning: passing 'char *' to parameter of
 type 'const unsigned char *' converts between
 pointers to integer types with different sign

 might_be_header takes a pointer to unsigned char.  However there is
 no reason for this to be the case.  The patch bellow fixes that.  In
 addition it adds 'const' to the type of 'line'.

If the only problem is the call to isupper(), then one easy and
useful solution is to define a macro called isupperch(), and use
that instead.  I've come across code in many different projects
where a programmer blindly passes a (char *) into the is*()
functions, and in some cases that mistake has caused real run-time
bugs.  It isn't just a dumb warning from an over-zealous compiler.
And the real run-time bugs may not show up on a given hardware
platform, depending on whether 'plain char' is the same as
'signed char' or 'unsigned char'.  So even programmers who have
exhaustively tested their code may not see any run-time bugs
depending on which platform they are testing on.

In any case, here are examples of what I use for macros:

/*
 * All the following take a parameter of 'int', but expect values in the
 * range of unsigned char.  Define wrappers which take values of type 'char',
 * whether signed or unsigned, and ensure they end up in the right range.
 */
#define isdigitch(Anychar) isdigit((u_char)(Anychar))
#define islowerch(Anychar) islower((u_char)(Anychar))
#define isupperch(Anychar) isupper((u_char)(Anychar))
#define tolowerch(Anychar) tolower((u_char)(Anychar))

-- 
Garance Alistair Drosehn= dro...@rpi.edu
Senior Systems Programmer   or   g...@freebsd.org
Rensselaer Polytechnic Institute;  Troy NY

Small patch for fmt(1)

2013-11-18 Thread Eitan Adler 
Hi all,

clang gives a warning when building fmt(1):

fmt.c:400:35: warning: passing 'char *' to parameter of type 'const
unsigned char *' converts between pointers to integer types with
different sign

might_be_header takes a pointer to unsigned char.  However there is no
reason for this to be the case.  The patch bellow fixes that. In
addition it adds 'const' to the type of 'line'.

[ http://people.freebsd.org/~eadler/files/o/0003.fmt.diff in case the
patch below gets mangled ]
Index: fmt.c
===
RCS file: /cvs/src/usr.bin/fmt/fmt.c,v
retrieving revision 1.29
diff -u -r1.29 fmt.c
--- fmt.c 17 Jan 2012 04:26:28 - 1.29
+++ fmt.c 19 Nov 2013 03:11:08 -
@@ -233,7 +233,7 @@
 static void process_named_file(const char *);
 static void process_stream(FILE *, const char *);
 static size_t indent_length(const char *, size_t);
-static int might_be_header(const unsigned char *);
+static int might_be_header(const char *);
 static void new_paragraph(size_t, size_t);
 static void output_word(size_t, size_t, const char *, size_t, size_t);
 static void output_indent(size_t);
@@ -385,7 +385,7 @@
  HdrType header_type;

  /* ^-- header_type of previous line; -1 at para start */
- char *line;
+ const char *line;
  size_t length;

  if (centerP) {
@@ -486,7 +486,7 @@
  * conservative to avoid mangling ordinary civilised text.
  */
 static int
-might_be_header(const unsigned char *line)
+might_be_header(const char *line)
 {

  if (!isupper(*line++))



-- 
Eitan Adler

Re: Small patch for fmt(1)

2013-11-18 Thread Philip Guenther 
On Mon, Nov 18, 2013 at 7:16 PM, Eitan Adler li...@eitanadler.com wrote:
 clang gives a warning when building fmt(1):

 fmt.c:400:35: warning: passing 'char *' to parameter of type 'const
 unsigned char *' converts between pointers to integer types with
 different sign

 might_be_header takes a pointer to unsigned char.  However there is no
 reason for this to be the case.  The patch bellow fixes that. In
 addition it adds 'const' to the type of 'line'.
...
 @@ -486,7 +486,7 @@
   * conservative to avoid mangling ordinary civilised text.
   */
  static int
 -might_be_header(const unsigned char *line)
 +might_be_header(const char *line)
  {

   if (!isupper(*line++))

The argument passed to isupper must be The argument to isupper() must
be EOF or representable as an unsigned char; otherwise, the result is
undefined.  That's guaranteed by the current version, but the diff
breaks that, so no, this isn't right, unless I missed a check
somewhere that tosses all negative chars...


Philip Guenther

Re: Small patch for fmt(1)

2013-11-18 Thread Eitan Adler 
On Mon, Nov 18, 2013 at 11:09 PM, Philip Guenther guent...@gmail.com wrote:
 On Mon, Nov 18, 2013 at 7:16 PM, Eitan Adler li...@eitanadler.com wrote:

 The argument passed to isupper must be The argument to isupper() must
 be EOF or representable as an unsigned char; otherwise, the result is
 undefined.  That's guaranteed by the current version, but the diff
 breaks that, so no, this isn't right, unless I missed a check
 somewhere that tosses all negative chars...

Of similar interest: get_line assigns the a value representable by an
unsigned char to a buffer of type 'char*'.

The following diff avoids the issue while also fixing the warning. An
alternative fix would be to change the intermediate 'char* 's to
'unsigned char*'.

Index: fmt.c
===
RCS file: /cvs/src/usr.bin/fmt/fmt.c,v
retrieving revision 1.29
diff -u -r1.29 fmt.c
--- fmt.c 17 Jan 2012 04:26:28 - 1.29
+++ fmt.c 19 Nov 2013 06:12:28 -
@@ -233,7 +233,7 @@
 static void process_named_file(const char *);
 static void process_stream(FILE *, const char *);
 static size_t indent_length(const char *, size_t);
-static int might_be_header(const unsigned char *);
+static int might_be_header(const char *);
 static void new_paragraph(size_t, size_t);
 static void output_word(size_t, size_t, const char *, size_t, size_t);
 static void output_indent(size_t);
@@ -385,7 +385,7 @@
  HdrType header_type;

  /* ^-- header_type of previous line; -1 at para start */
- char *line;
+ const char *line;
  size_t length;

  if (centerP) {
@@ -486,14 +486,14 @@
  * conservative to avoid mangling ordinary civilised text.
  */
 static int
-might_be_header(const unsigned char *line)
+might_be_header(const char *line)
 {

- if (!isupper(*line++))
+ if (!isupper((unsigned char)*line++))
  return 0;
- while (isalnum(*line) || *line == '-')
+ while (isalnum((unsigned char)*line) || *line == '-')
  ++line;
- return (*line == ':'  isspace(line[1]));
+ return (*line == ':'  isspace((unsigned char)line[1]));
 }

 /* Begin a new paragraph with an indent of |indent| spaces.





-- 
Eitan Adler