X-Authentication-Warning: delorie.com: mailnull set sender to djgpp-workers-bounces using -f
Sender: rich AT phekda DOT freeserve DOT co DOT uk
Message-ID: <3C29CCF7.7F3723F3@phekda.freeserve.co.uk>
Date: Wed, 26 Dec 2001 13:13:27 +0000
From: Richard Dawe <rich AT phekda DOT freeserve DOT co DOT uk>
X-Mailer: Mozilla 4.77 [en] (X11; U; Linux 2.2.19 i586)
X-Accept-Language: de,fr
MIME-Version: 1.0
To: djgpp-workers AT delorie DOT com
Subject: Re: regcomp NLS fix
References: <6F32F7D2 DOT 12300D8B DOT 09ACFA57 AT netscape DOT net>
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Reply-To: djgpp-workers AT delorie DOT com

Hello.

Alexander Aganichev wrote:
[snip]
> I have redone the patch (regex.nls+warning.diff) by casting all ctype.h
> function's arguments to unsigned char. Also I have fix the warnings
> caused by the missed brackets and unused values calculated. There's 4
> more warnings in pedantic mode about possible use of uninitialized
> variables, but they seems to be safe to ignore.
>
> The second fix (regex.nls2.diff) is intended for the proper working in
> NLS capable environment.

I don't know much about NLS, but I have a few comments about these
patches:

* Could you use the '-p' option, when you do a diff please? That includes
the function name in the header of each diff, which is helpful when
looking at the code (for me, at least).

* Why do you cast int characters to unsigned char, when passing them to
the ctype functions? Is this to ensure that the inline implementation of
the ctype functions doesn't overrun the ctype buffers by looking up an
invalid character (see include/inlines/ctype.ha)? Does the code without
this patch crash because a buffer overrun?

* What do you mean by "proper working in NLS capable environment"? It
looks like the code still only supports the C locale. Presumably you mean
it doesn't crash, when locales other than the C one are being used by
programs built against libc?

These last two aren't criticisms. I just don't really understand what the
patch is trying to fix.

[snip]
> --- regex.orig/regcomp.c    Sat Jun  9 23:50:56 2001
> +++ regex/regcomp.c Tue Dec 18 18:51:12 2001
> @@ -53,10 +53,10 @@
>  #define    NEXTn(n)    (p->next += (n))
>  #define    GETNEXT()   (*p->next++)
>  #define    SETERROR(e) seterr(p, (e))
> -#define    REQUIRE(co, e)  ((co) || SETERROR(e))
> -#define    MUSTSEE(c, e)   (REQUIRE(MORE() && PEEK() == (c), e))
> -#define    MUSTEAT(c, e)   (REQUIRE(MORE() && GETNEXT() == (c), e))
> -#define    MUSTNOTSEE(c, e)    (REQUIRE(!MORE() || PEEK() != (c), e))
> +#define    REQUIRE(co, e)  { if(!(co)) SETERROR(e); }
> +#define    MUSTSEE(c, e)   REQUIRE(MORE() && PEEK() == (c), e)
> +#define    MUSTEAT(c, e)   REQUIRE(MORE() && GETNEXT() == (c), e)
> +#define    MUSTNOTSEE(c, e)    REQUIRE(!MORE() || PEEK() != (c), e)
>  #define    EMIT(op, sopnd) doemit(p, (sop)(op), (size_t)(sopnd))
>  #define    INSERT(op, pos) doinsert(p, (sop)(op), HERE()-(pos)+1, pos)
>  #define    AHEAD(pos)      dofwd(p, pos, HERE()-(pos))

Why did you remove the brackets around REQUIRE(...) in MUSTSEE, MUSTEAT
and MUSTNOTSEE? Since they're macros, isn't it safer to enclose them in
brackets, to avoid subtle bugs?

Thanks, bye, Rich =]

-- 
Richard Dawe [ http://www.phekda.freeserve.co.uk/richdawe/ ]