Re: [PATCH nd/wildmatch] Correct Git's version of isprint and isspace

[Johannes Sixt <j6t@kdbg.org>]

Am 13.11.2012 11:46, schrieb Nguyễn Thái Ngọc Duy:
> @@ -14,11 +14,11 @@ enum {
>  	P = GIT_PATHSPEC_MAGIC, /* other non-alnum, except for ] and } */
>  	X = GIT_CNTRL,
>  	U = GIT_PUNCT,
> -	Z = GIT_CNTRL | GIT_SPACE
> +	Z = GIT_CNTRL_SPACE
>  };
>  
> -const unsigned char sane_ctype[256] = {
> -	X, X, X, X, X, X, X, X, X, Z, Z, X, X, Z, X, X,		/*   0.. 15 */
> +const unsigned int sane_ctype[256] = {
> +	X, X, X, X, X, X, X, X, X, Z, Z, Z, Z, Z, X, X,		/*   0.. 15 */
>  	X, X, X, X, X, X, X, X, X, X, X, X, X, X, X, X,		/*  16.. 31 */
>  	S, P, P, P, R, P, P, P, R, R, G, R, P, P, R, P,		/*  32.. 47 */
>  	D, D, D, D, D, D, D, D, D, D, P, P, P, P, P, G,		/*  48.. 63 */
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 02f48f6..4ed3f94 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -474,8 +474,8 @@ extern const char tolower_trans_tbl[256];
>  #undef ispunct
>  #undef isxdigit
>  #undef isprint
> -extern const unsigned char sane_ctype[256];
> -#define GIT_SPACE 0x01
> +extern const unsigned int sane_ctype[256];
> +#define GIT_CNTRL_SPACE 0x01
>  #define GIT_DIGIT 0x02
>  #define GIT_ALPHA 0x04
>  #define GIT_GLOB_SPECIAL 0x08
> @@ -483,9 +483,10 @@ extern const unsigned char sane_ctype[256];
>  #define GIT_PATHSPEC_MAGIC 0x20
>  #define GIT_CNTRL 0x40
>  #define GIT_PUNCT 0x80
> -#define sane_istest(x,mask) ((sane_ctype[(unsigned char)(x)] & (mask)) != 0)
> +#define GIT_SPACE 0x100
> +#define sane_istest(x,mask) ((sane_ctype[(unsigned int)(x)] & (mask)) != 0)
>  #define isascii(x) (((x) & ~0x7f) == 0)
> -#define isspace(x) sane_istest(x,GIT_SPACE)
> +#define isspace(x) sane_istest(x,GIT_SPACE | GIT_CNTRL_SPACE)
>  #define isdigit(x) sane_istest(x,GIT_DIGIT)
>  #define isalpha(x) sane_istest(x,GIT_ALPHA)
>  #define isalnum(x) sane_istest(x,GIT_ALPHA | GIT_DIGIT)
> @@ -493,7 +494,7 @@ extern const unsigned char sane_ctype[256];
>  #define isupper(x) sane_iscase(x, 0)
>  #define is_glob_special(x) sane_istest(x,GIT_GLOB_SPECIAL)
>  #define is_regex_special(x) sane_istest(x,GIT_GLOB_SPECIAL | GIT_REGEX_SPECIAL)
> -#define iscntrl(x) (sane_istest(x,GIT_CNTRL))
> +#define iscntrl(x) (sane_istest(x,GIT_CNTRL | GIT_CNTRL_SPACE))
>  #define ispunct(x) sane_istest(x, GIT_PUNCT | GIT_REGEX_SPECIAL | \
>  		GIT_GLOB_SPECIAL | GIT_PATHSPEC_MAGIC)
>  #define isxdigit(x) (hexval_table[x] != -1)

So we have two properties that overlap:

      SSSSSSSSSS
   CCCCCCCC

You seem to generate partions:

   XXXYYYYYZZZZZ

then assign individual bits to each partition. Now each entry in the
lookup table has only one bit set. Then you define isxxx() to check for
one of the two possible bits:

   iscntrl is X or Y
   isspace is Y or Z

But shouldn't you just assign one bit for S and another one for C, have
entries in the lookup table with more than one bit set, and check for
only one bit in the isxxx macro?

That way you don't run out of bits as easily as you do with this patch.

-- Hannes