Re: [PATCH v2] __builtin_expect cleanup for iconv{,data}/*.c

[Kalle Olavi Niemitalo <kon at iki dot fi>]

Florian Weimer <fweimer@redhat.com> writes:

> --- a/iconv/gconv_simple.c
> +++ b/iconv/gconv_simple.c
> @@ -387,8 +387,7 @@ ucs4_internal_loop_single (struct __gconv_step *step,
>        return __GCONV_INCOMPLETE_INPUT;
>      }
>  
> -  if (__builtin_expect (((unsigned char *) state->__value.__wchb)[0] > 0x80,
> -			0))
> +  if (__glibc_unlikely (((unsigned char *) state->__value.__wchb)[0] > 0x80))

I think that is a bug in the original code.  Should compare > 0x7f.
Likewise in internal_ucs4_loop_unaligned.
Contrast to ucs4_internal_loop, which compares inval > 0x7fffffff.

> -  if (__builtin_expect (flags & GCONV_AVOID_NOCONV, 0)
> +  if (__glibc_unlikely (flags & GCONV_AVOID_NOCONV)

This looks a bit suspicious because it is not obvious that the
flags & GCONV_AVOID_NOCONV expression always evaluates to 0 or 1.
I'm not sure __glibc_unlikely ((flags & GCONV_AVOID_NOCONV) != 0)
would be any nicer, though.

My concern was that the code is not very different from
__builtin_expect (x & 2, 1), from which an excessively clever
compiler could figure out that the expectation cannot ever hold.
Anyway, the code is different enough to avoid any such problem.

> -	    /*if (__builtin_expect (ch, 0) == __UNKNOWN_10646_CHAR)*/	      \
> +	    /*if (__glibc_unlikely (ch == __UNKNOWN_10646_CHAR))*/	      \
>  	    if (__glibc_unlikely (ch == __UNKNOWN_10646_CHAR))		      \

I don't think that comment has any value left.

I didn't finish reading the patch yet.