Re: [PATCH v3 1/5] Copy utf-7 module to modified-utf-7

[Max Gautier <mg@max.gautier.name>]

* Florian Weimer via Libc-alpha:
> Given that UTF-7 conversion (either variant) is not
> performance-critical, I suggest to have just one implementation file.
> 
> You can use step->__data to keep track of which variant is active.  See
> iconvdata/iso646.c for an example.  There is no need to allocate a
> separate object; you can store the flag directly in the __data member.

I'll work on that.
I might use some advice on specific parts. About the classification of
characters (direct or not etc) :
- utf-7.c
+ utf-7-imap.c
> -static const unsigned char direct_tab[128 / 8] =
> -  {
> -    0x00, 0x26, 0x00, 0x00, 0x81, 0xf3, 0xff, 0x87,
> -    0xfe, 0xff, 0xff, 0x07, 0xfe, 0xff, 0xff, 0x07
> -  };
> -
>  static int
>  isdirect (uint32_t ch)
>  {
> -  return (ch < 128 && ((direct_tab[ch >> 3] >> (ch & 7)) & 1));
> -}
> -
> -
> -/* The set of "direct and optional direct characters":
> -   A-Z a-z 0-9 ' ( ) , - . / : ? space tab lf cr
> -   ! " # $ % & * ; < = > @ [ ] ^ _ ` { | }
> -*/
> -
> -static const unsigned char xdirect_tab[128 / 8] =
> -  {
> -    0x00, 0x26, 0x00, 0x00, 0xff, 0xf7, 0xff, 0xff,
> -    0xff, 0xff, 0xff, 0xef, 0xff, 0xff, 0xff, 0x3f
> -  };
> -
> -static int
> -isxdirect (uint32_t ch)
> -{
> -  return (ch < 128 && ((xdirect_tab[ch >> 3] >> (ch & 7)) & 1));
> +  return ((ch == '\n' || ch == '\t' || ch == '\r')
> +		  || (ch >= 0x20 && ch <= 0x7e && ch != '&'));
>  }
>  
> -
> -/* The set of "extended base64 characters":
> -   A-Z a-z 0-9 + / -
> +/* The set of "modified base64 characters":
> +   A-Z a-z 0-9 + , -
>  */
>  
> -static const unsigned char xbase64_tab[128 / 8] =
> -  {
> -    0x00, 0x00, 0x00, 0x00, 0x00, 0xa8, 0xff, 0x03,
> -    0xfe, 0xff, 0xff, 0x07, 0xfe, 0xff, 0xff, 0x07
> -  };
> -
>  static int
> -isxbase64 (uint32_t ch)
> +ismbase64 (uint32_t ch)
>  {
> -  return (ch < 128 && ((xbase64_tab[ch >> 3] >> (ch & 7)) & 1));
> +  return ((ch >= 'a' && ch <= 'z')
> +			  || (ch >= 'A' && ch <= 'Z')
> +			  || (ch >= '0' && ch <= '9')
> +			  || (ch == '+' || ch == ','));
>  }

When I initially looked at utf-7.c, the use of the _tab arrays with
magic values and the subsequent shifting didn't make a lot of sense to
me, which is why I modified them like this for utf-7-imap. If they are
in the same file, it's probably better to use the same method.
So do you see any benefits to keeping the old method ?
Testing directly for the actual characters seems a lot more readable to
me, is shorter, and can be mapped to the RFC definition.

I didn't find the reason for using the current method by looking at the
git history. The only one I could think of is performance, but I don't
see how or what it would improve. If someone has some hints, let me
know.

 
> You could remove the UTF7_ENCODE_OPTIONAL_CHARS from the existing UTF-7
> codec in a first, separate patch.

Do you mean I should modify the utf-7 conversion to not encode the
optional chars ?  That would change the result of utf-7 conversions,
wouldn't it ? I'm not opposed to it, but isn't that going to break
things ?

Thanks

-- 
Max Gautier
mg@max.gautier.name