Re: [PATCH] binutils: Use loops in `byte_get_*_endian` functions instead of switch cases.

On 06.07.2026 18:59, Julien Thillard wrote:
> Switch cases are used to read a value with a given size, but
> these functions could be rewritten using a small loop instead,
> avoiding code repetition.
> This is already done in `byte_put_*_endian` functions.
> 
> binutils:
> 
>         * elfcomm.c: Use loop instead of switch case.

While the change looks technically correct (apart from ...

> --- a/binutils/elfcomm.c
> +++ b/binutils/elfcomm.c
> @@ -114,127 +114,35 @@ uint64_t (*byte_get) (const unsigned char *, unsigned int);
>  uint64_t
>  byte_get_little_endian (const unsigned char *field, unsigned int size)
>  {
> -  switch (size)
> -    {
> -    case 1:
> -      return *field;
> -
> -    case 2:
> -      return ((uint64_t) field[0]
> -	      | ((uint64_t) field[1] << 8));
> -
> -    case 3:
> -      return ((uint64_t) field[0]
> -	      | ((uint64_t) field[1] << 8)
> -	      | ((uint64_t) field[2] << 16));
> -
> -    case 4:
> -      return ((uint64_t) field[0]
> -	      | ((uint64_t) field[1] << 8)
> -	      | ((uint64_t) field[2] << 16)
> -	      | ((uint64_t) field[3] << 24));
> -
> -    case 5:
> -      return ((uint64_t) field[0]
> -	      | ((uint64_t) field[1] << 8)
> -	      | ((uint64_t) field[2] << 16)
> -	      | ((uint64_t) field[3] << 24)
> -	      | ((uint64_t) field[4] << 32));
> +  unsigned int i;
> +  uint64_t read = 0;
>  
> -    case 6:
> -      return ((uint64_t) field[0]
> -	      | ((uint64_t) field[1] << 8)
> -	      | ((uint64_t) field[2] << 16)
> -	      | ((uint64_t) field[3] << 24)
> -	      | ((uint64_t) field[4] << 32)
> -	      | ((uint64_t) field[5] << 40));
> -
> -    case 7:
> -      return ((uint64_t) field[0]
> -	      | ((uint64_t) field[1] << 8)
> -	      | ((uint64_t) field[2] << 16)
> -	      | ((uint64_t) field[3] << 24)
> -	      | ((uint64_t) field[4] << 32)
> -	      | ((uint64_t) field[5] << 40)
> -	      | ((uint64_t) field[6] << 48));
> -
> -    case 8:
> -      return ((uint64_t) field[0]
> -	      | ((uint64_t) field[1] << 8)
> -	      | ((uint64_t) field[2] << 16)
> -	      | ((uint64_t) field[3] << 24)
> -	      | ((uint64_t) field[4] << 32)
> -	      | ((uint64_t) field[5] << 40)
> -	      | ((uint64_t) field[6] << 48)
> -	      | ((uint64_t) field[7] << 56));
> -
> -    default:
> +  if (size > sizeof (uint64_t))
> +    {
>        error (_("Unhandled data length: %d\n"), size);
>        abort ();
>      }
> +  for(i = 0; i < size; i++)

... a style issue here [missing blank after "for"] and again in the other
loop), how does generated code change? There may have been a reason things
were coded this way.

There's also a subtle, possibly (but not necessarily) benign change in
behavior: size being 0 previously hit the default: label, while now we'd
silently return 0 in that case (which may be correct in some cases, but
which may not be desired in others).

Maybe this as a compromise, reducing code size while retaining original
properties?

uint64_t
byte_get_little_endian (const unsigned char *field, unsigned int size)
{
  uint64_t read = 0;

  switch (size)
    {
    case 8:
      read |= (uint64_t) field[7] << 56;
      /* Fall through.  */
    case 7:
      read |= (uint64_t) field[6] << 48;
      /* Fall through.  */
    case 6:
      read |= (uint64_t) field[5] << 40;
      /* Fall through.  */
    case 5:
      read |= (uint64_t) field[4] << 32;
      /* Fall through.  */
    case 4:
      read |= (uint64_t) field[3] << 24;
      /* Fall through.  */
    case 3:
      read |= (uint64_t) field[2] << 16;
      /* Fall through.  */
    case 2:
      read |= (uint64_t) field[1] <<  8;
      /* Fall through.  */
    case 1:
      return read | *field;

    default:
      error (_("Unhandled data length: %u\n"), size);
      abort ();
    }
}

Jan