Re: [PATCH v4 3/4] iconv: make utf-7.c able to use variants

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 12/03/2022 08:07, Max Gautier wrote:
> On Mon, Mar 07, 2022 at 09:34:47AM -0300, Adhemerval Zanella wrote:
>>>  static int
>>> -isxdirect (uint32_t ch)
>>> +isxdirect (uint32_t ch, enum variant var)
>>>  {
>>> -    return (ch == '\t'
>>> -            || ch == '\n'
>>> -            || ch == '\r'
>>> -            || (between(ch, ' ','}')
>>> -                && ch != '+' && ch != '\\')
>>> -           );
>>> +    return(isdirect(ch, var)
>>> +            || (var == UTF7 &&
>>> +                (between(ch, '!', '&')
>>> +                 || ch == '*'
>>> +                 || between(ch, ';', '@')
>>> +                 || (between(ch, '[', '`') && ch != '\\')
>>> +                 || between(ch, '{', '}'))
>>> +                )
>>> +          );
>>>  }
>>>  
>>
>> The change is ok, but maybe adding the variant out makes it more clear:
>>
>>    if (var != UTF7)
>>      return 0;
>>    [...]
>>
> something like this ? 
> 
> if (isdirect(ch, var))
>    return true;
> if (var != UTF7)
>    return false;
> return (between(ch, '!', '&')
>   || ch == '*'
>   || between(ch, ';', '@')
>   || (between(ch, '[', '`') && ch != '\\')
>   || between(ch, '{', '}'))
>  );
> 

Yes, it is slight better for readability (don't forget the space before '('
and the extra parenthesis in return is not required). 

> I'd prefer the single expression form, but that works too.
> 
>> Also I think since you change it, it would be better to use 'bool' as
>> return type.
> 
> Ok. I was not sure whether it was ok to use bool type or not.

We build internally with gnu11, so we can use most of c11 facilities (there
are some spots like atomics that we are still migrating and require extra
care to not call libatomics).

> 
> Thanks for the review.
>