Re: several trim bugs

[Bruno Haible]

Colin Watson wrote:
> Firstly, the trim module fails to add its files to lib_SOURCES, so they
> don't get built.
> 
> Secondly, after fixing that, it tries to include "mbuiter.h" without
> depending on that module. On inspection, I found that it claims to be
> including it for MB_CUR_MAX, but that's actually in <stdlib.h> (the only
> system header mbuiter.h includes over and above mbiter.h) so including
> "mbuiter.h" for it seems odd and redundant. I added a check for
> defined(MB_CUR_MAX) because I don't know offhand if C89 <stdlib.h>
> guarantees that; if it does, please remove this check again.
> 
> Thirdly, trim.c uses isspace unconditionally so it should also include
> <ctype.h> unconditionally.

Thanks for reporting all this. I'm applying the appended patch to fix
all this.

> Fourthly, gcc complains:
> 
>   trim.c: In function ‘trim2’:
>   trim.c:67: warning: ‘r’ may be used uninitialized in this function
> 
> ... and it may be that the state machine in fact renders it impossible
> for this to be used uninitialised but this wasn't obvious to me.

I added a comment with the invariant.

> Some more sanity-checking seems to be called for here.

I don't agree. The code is fine. Some day gcc will learn not to emit
"uninitialized" warnings for cases like this. The code that you put in
makes the code flow needlessly more convoluted.

Bruno


2007-10-18  Colin Watson <address@hidden>  (tiny change)
            Bruno Haible  <address@hidden>

        * lib/trim.c: Include config.h unconditionally. Include trim.h always.
        Include ctype.h always. Include stdlib.h, not mbuiter.h, for MB_CUR_MAX.
        * modules/trim (Depends-on): Add mbchar.
        (configure.ac): Add gl_FUNC_MBRTOWC.
        (Makefile.am): Augment lib_SOURCES.

--- lib/trim.c.orig     2007-10-18 03:47:05.000000000 +0200
+++ lib/trim.c  2007-10-18 03:28:14.000000000 +0200
@@ -1,5 +1,5 @@
 /* Removes leading and/or trailing whitespaces
-   Copyright (C) 2006 Free Software Foundation, Inc.
+   Copyright (C) 2006-2007 Free Software Foundation, Inc.
 
    This program is free software: you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -16,21 +16,21 @@
 
 /* Written by Davide Angelocola <address@hidden> */
 
-#ifdef HAVE_CONFIG_H
-# include <config.h>
-#endif
+#include <config.h>
+
+/* Specification.  */
+#include "trim.h"
+
+#include <ctype.h>
 
 #if HAVE_MBRTOWC 
 # include <stddef.h>
+# include <stdlib.h>
 # include "mbchar.h"
 # include "mbiter.h"
-# include "mbuiter.h"          /* FIXME: for MB_CUR_MAX */
-#else
-# include <ctype.h>
 #endif
 
 #include "xalloc.h"
-#include "trim.h"
 
 char *
 trim2(const char *s, int how)
@@ -62,7 +62,7 @@
       if (how != TRIM_LEADING) 
        {
          int state = 0;
-         char *r;
+         char *r; /* used only while state = 2 */
          
          mbi_init (i, d, strlen (d));
 
--- modules/trim.orig   2007-10-18 03:47:05.000000000 +0200
+++ modules/trim        2007-10-18 03:23:06.000000000 +0200
@@ -2,16 +2,19 @@
 trim() removes leading and/or trailing whitespaces
 
 Files:
-lib/trim.c
 lib/trim.h
+lib/trim.c
 
 Depends-on:
 xalloc
+mbchar
 mbiter
 
 configure.ac:
+gl_FUNC_MBRTOWC
 
 Makefile.am:
+lib_SOURCES += trim.c
 
 Include:
 #include "trim.h"