Mail Archives: djgpp-workers/2001/12/26/08:14:22
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/ ]
- Raw text -