Generically convert bytes to integers

Question

I wanted to know If there was a way to make it more versatile or shorter/simpler. Here's the code:

// Take only std::byte parameter and return an unsigned integral
constexpr auto bytes_to_uint(std::same_as<std::byte> auto... bytes) -> std::unsigned_integral auto {
    constexpr auto N = sizeof...(bytes);

    // Integral types large enough to hold N bytes
    using types = std::tuple<
        std::uint8_t,
        std::uint16_t,
        std::uint32_t,
        std::uint32_t,
        std::uint64_t,
        std::uint64_t,
        std::uint64_t,
        std::uint64_t
    >;
    
    using result = std::tuple_element_t<N, types>;
    
    return [&]<std::size_t... S>(std::index_sequence<S...>) {
        // Accumulate the part of the number using the bitwise or operator for each bytes
        return ((static_cast<result>(bytes) << CHAR_BIT * (N - S - 1)) | ... );
    }(std::make_index_sequence<N>{});
}

It is meant to be used like this:

bytes_to_uint(std::byte{0xaa}, std::byte{0xbb}); // std::uint16_t: 0xaabb

bytes_to_uint(
    std::byte{0x11},
    std::byte{0x22},
    std::byte{0x33},
    std::byte{0x44}
); // std::uint32_t: 0x11223344

Answer 1

The function signature strikes me as difficult to read, thanks to the constraint std::same_as<std::byte> auto... and the trailing "return type" std::unsigned_integral auto. I might rather write something like

constexpr auto bytes_to_uint(std::initializer_list<std::byte> bytes) {

...Ah, but then you couldn't use bytes.size() as a constant expression; I see. So I'd think about writing an overload set, like this:

constexpr std::uint8_t bytes_to_uint(std::byte a) {
    return a;
}
constexpr std::uint16_t bytes_to_uint(std::byte a, std::byte b) {
    return (a << 8) | b;
}
constexpr std::uint32_t bytes_to_uint(std::byte a, std::byte b, std::byte c, std::byte d) {
    return (a << 24) | (b << 16) | (c << 8) | d;
}

But I guess this is messy because you need 16 different overloads. You can't even use default function arguments, because you want bytes_to_uint(a,b,c) to be equal to bytes_to_uint(0,a,b,c) and not bytes_to_uint(a,b,c,0). Of course you could still write

#define B std::byte
constexpr std::uint8_t bytes_to_uint(B a)
  { return bytes_to_uint(0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,a); }
constexpr std::uint16_t bytes_to_uint(B a, B b)
  { return bytes_to_uint(0,0,0,0,0,0,0,0,0,0,0,0,0,0,a,b); }
constexpr std::uint32_t bytes_to_uint(B a, B b, B c)
  { return bytes_to_uint(0,0,0,0,0,0,0,0,0,0,0,0,0,a,b,c); }
constexpr std::uint32_t bytes_to_uint(B a, B b, B c, B d)
  { return bytes_to_uint(0,0,0,0,0,0,0,0,0,0,0,0,a,b,c,d); }
constexpr std::uint64_t bytes_to_uint(B a, B b, B c, B d, B e)
  { return bytes_to_uint(0,0,0,0,0,0,0,0,0,0,0,a,b,c,d,e); }
[...22 more lines...]
#undef B

but I bet you don't want to do that. Okay, let's stick with the templatey thing you did.

---

using result = std::tuple_element_t<N, types>;

Preferiría ver este uso de typedef dependiente CamelCase(como un parámetro de plantilla) o suffixedwith_type(como un typedef de miembro STL). Llamarlo resulthace que se parezca demasiado a una variable y dificulta elegir el lugar donde lo usa.

En lugar de gastar 13 líneas y una <tuple>dependencia, prefiero hacer

using ResultType = std::conditional_t<
    (N == 1), std::uint8_t, std::conditional_t<
    (N == 2), std::uint16_t, std::conditional_t<
    (N <= 4), std::uint32_t, std::uint64_t>>>;

Lo que me recuerda, necesitas algo como

static_assert(N <= 16);

para evitar que intente manejar una lista de argumentos de 17 bytes o más.

Y ni siquiera me di cuenta hasta que lo probé en Godbolt, ¡pero aquí tienes un error! tuple_element_t<N-1, types>quisiste decir Recuerde que la indexación siempre comienza en cero (en todas partes excepto en <regex>).

---

Si no te gusta conditional_t, otra opción es usar ifs simples. Factorice el cálculo mientras aún está parametrizado ResultTypey luego use una if-elsecadena para decidir el tipo correcto para conectarlo ResultTypemás tarde:

auto do_it = [&]<class ResultType, std::size_t... S>(ResultType, std::index_sequence<S...>) {
    return ((static_cast<ResultType>(bytes) << CHAR_BIT * (N - S - 1)) | ... );
};
if constexpr (N == 1) {
    return do_it(std::uint8_t{}, std::make_index_sequence<N>{});
} else if constexpr (N == 2) {
    return do_it(std::uint16_t{}, std::make_index_sequence<N>{});
} else if constexpr (N <= 4) {
    return do_it(std::uint32_t{}, std::make_index_sequence<N>{});
} else if constexpr (N <= 8) {
    return do_it(std::uint64_t{}, std::make_index_sequence<N>{});
}

---

Aún mejor, permita que el compilador haga los cálculos en su ancho de bits preferido (que es de 64 bits en todas las plataformas de escritorio que me interesan) y luego trunque al final. Esto produce código similar y se lee mejor aún:

std::uint64_t result = [&]<std::size_t... S>( std::index_sequence<S...>) {
    return ((static_cast<std::uint64_t>(bytes) << CHAR_BIT * (N - S - 1)) | ... );
}(std::make_index_sequence<N>{});
if constexpr (N == 1) {
    return std::uint8_t(result);
} else if constexpr (N == 2) {
    return std::uint16_t(result);
} else if constexpr (N <= 4) {
    return std::uint32_t(result);
} else {
    return std::uint64_t(result);
}

---

Tienes otro error cuando N == 1(además del error de apagado por uno). Cuando N == 1, la expresión de pliegue no tiene ninguna |operación, por lo que solo se uint8_tdesplaza por cero. Esa expresión de cambio tiene tipoint . Que no es un tipo integral sin signo. ¡Entonces su restricción de tipo de retorno falla!

Esta es solo otra razón para hacer todos los cálculos primero en uint64_t, y luego bajar a la uint8_tderecha antes de regresar, como se muestra en mi último ejemplo anterior.

Escribir cualquier caso de prueba habría detectado tanto este error como el error de apagado por uno. ¡Los casos de prueba siempre son importantes! Especialmente cuando está planeando poner el código para revisión pública. (O para revisión de compañeros de trabajo, para el caso).

---

Finalmente, recomiendo paréntesis para aclarar la precedencia de x << CHAR_BIT * y. En contexto, es obvio cuál esperaba que fuera la precedencia; pero como lector, no estoy seguro de que tengas razón. Pon los paréntesis para que no tenga que pensarlo ni un segundo.

Sin embargo, en este contexto, ese es un punto muy pequeño, porque claramente no espera que nadie lea la expresión ((static_cast<result>(bytes) << CHAR_BIT * (N - S - 1)) | ... ). Es una línea de código de "confía en mí".

También es una tontería pretender que eso CHAR_BITes relevante aquí. Este código explota espectacularmente si CHAR_BITes otra cosa que no sea 8. Así que solo escribe 8; y si se ve obligado a trabajar en una referencia a CHAR_BIT, hágalo escribiendo

static_assert(CHAR_BIT == 8);

en la parte superior de la función.