Skip to main content
added 154 characters in body
Source Link
G. Sliepen
  • 69.3k
  • 3
  • 75
  • 180

In any case, make sure you don't mix unsigned char and std::uint8_t like you did in your code: I recommend you stick with unsigned char, shift by CHAR_BITS, and not use any of the fixed-width types in your Endian class. If you do want to swap 8-bits at a time, then the size of the array data is wrong for CHAR_BITS > 8, it should then be (sizeof(T) * CHAR_BITS) / 8.

In any case, make sure you don't mix unsigned char and std::uint8_t like you did in your code: I recommend you stick with unsigned char, shift by CHAR_BITS, and not use any of the fixed-width types in your Endian class.

In any case, make sure you don't mix unsigned char and std::uint8_t like you did in your code: I recommend you stick with unsigned char, shift by CHAR_BITS, and not use any of the fixed-width types in your Endian class. If you do want to swap 8-bits at a time, then the size of the array data is wrong for CHAR_BITS > 8, it should then be (sizeof(T) * CHAR_BITS) / 8.

deleted 1482 characters in body
Source Link
G. Sliepen
  • 69.3k
  • 3
  • 75
  • 180

About the design

I don't think this looks like a good design to me. In particular, I would expect:

#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
template<std::integral T>
using BigEndian = Endian<T, std::views::reverse, std::views::reverse>;

template<std::integral T>
using LittleEndian = Endian<T, std::views::all, std::views::all>;
#else
template<std::integral T>
using BigEndian = Endian<T, std::views::all, std::views::all>;

template<std::integral T>
using LittleEndian = Endian<T, std::views::reverse, std::views::reverse>;
#endif

This ensures that what you write into a value is the same as what you read from it, but the way it is stored in memory is in the given endianness. That also makes it easy to use it in a struct:

struct my_sockaddr {
    BigEndian<uint32_t> address;
    BigEndian<uint16_t> port;
    …
};

Then you could write something like:

my_sockaddr local_webserver = {0x7f'00'00'01, 80};
connect(fd, (sockaddr*)&local_web_server);

Or:

my_sockaddr peer_address;
accept(fd, (sockaddr*)&peer_address);

Missing tests

Your unit tests don't test whether the value is preserved when they make a round trip. I would expect at least something like:

TEST(big_endian_round_trip, uint32)
{
    std::uint32_t in = 0x12345678;
    auto be1 = BigEndian{x};
    auto be2 = decltype(be1){};
    std::memcpy(&be2, &be1, sizeof be1);
    std::uint32_t out = be2;
    EXPECT_EQ(in, out);
}

And a similar one for little_endian_round_trip of course.

About your concerns

In any case, make sure you don't mix unsigned char and std::uint8_t like you did in your code: I recommend you stick with unsigned char, shift by CHAR_BITS, and not use any of the fixed-width types in your Endian class.

At the other extreme I would say that in practice, you nowadays don't need to worry about anything but little-endian systems where integer sizes are multiples of 8. So you only need a BigEndian class to deal with conversions to/from big-endian, nothing more. Anything else falls into the YAGNI category.

Make it constexpr

You can make the constructor and conversion operator constexpr.

About the design

I don't think this looks like a good design to me. In particular, I would expect:

#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
template<std::integral T>
using BigEndian = Endian<T, std::views::reverse, std::views::reverse>;

template<std::integral T>
using LittleEndian = Endian<T, std::views::all, std::views::all>;
#else
template<std::integral T>
using BigEndian = Endian<T, std::views::all, std::views::all>;

template<std::integral T>
using LittleEndian = Endian<T, std::views::reverse, std::views::reverse>;
#endif

This ensures that what you write into a value is the same as what you read from it, but the way it is stored in memory is in the given endianness. That also makes it easy to use it in a struct:

struct my_sockaddr {
    BigEndian<uint32_t> address;
    BigEndian<uint16_t> port;
    …
};

Then you could write something like:

my_sockaddr local_webserver = {0x7f'00'00'01, 80};
connect(fd, (sockaddr*)&local_web_server);

Or:

my_sockaddr peer_address;
accept(fd, (sockaddr*)&peer_address);

Missing tests

Your unit tests don't test whether the value is preserved when they make a round trip. I would expect at least something like:

TEST(big_endian_round_trip, uint32)
{
    std::uint32_t in = 0x12345678;
    auto be1 = BigEndian{x};
    auto be2 = decltype(be1){};
    std::memcpy(&be2, &be1, sizeof be1);
    std::uint32_t out = be2;
    EXPECT_EQ(in, out);
}

And a similar one for little_endian_round_trip of course.

About your concerns

In any case, make sure you don't mix unsigned char and std::uint8_t like you did in your code: I recommend you stick with unsigned char and not use any of the fixed-width types in your Endian class.

At the other extreme I would say that in practice, you nowadays don't need to worry about anything but little-endian systems where integer sizes are multiples of 8. So you only need a BigEndian class to deal with conversions to/from big-endian, nothing more. Anything else falls into the YAGNI category.

About your concerns

In any case, make sure you don't mix unsigned char and std::uint8_t like you did in your code: I recommend you stick with unsigned char, shift by CHAR_BITS, and not use any of the fixed-width types in your Endian class.

At the other extreme I would say that in practice, you nowadays don't need to worry about anything but little-endian systems where integer sizes are multiples of 8. So you only need a BigEndian class to deal with conversions to/from big-endian, nothing more. Anything else falls into the YAGNI category.

Make it constexpr

You can make the constructor and conversion operator constexpr.

added 4 characters in body
Source Link
G. Sliepen
  • 69.3k
  • 3
  • 75
  • 180
TEST(big_endian_round_trip, uint32)
{
    std::uint32_t in = 0x12345678;
    auto be1 = BigEndian{x};
    auto be2 = BigEndiandecltype(be1){};
    std::memcpy(&be2, &be1, sizeof be1);
    std::uint32_t out = be2;
    EXPECT_EQ(in, out);
}
TEST(big_endian_round_trip, uint32)
{
    std::uint32_t in = 0x12345678;
    auto be1 = BigEndian{x};
    auto be2 = BigEndian{};
    std::memcpy(&be2, &be1, sizeof be1);
    std::uint32_t out = be2;
    EXPECT_EQ(in, out);
}
TEST(big_endian_round_trip, uint32)
{
    std::uint32_t in = 0x12345678;
    auto be1 = BigEndian{x};
    auto be2 = decltype(be1){};
    std::memcpy(&be2, &be1, sizeof be1);
    std::uint32_t out = be2;
    EXPECT_EQ(in, out);
}
added 459 characters in body
Source Link
G. Sliepen
  • 69.3k
  • 3
  • 75
  • 180
Loading
added 826 characters in body
Source Link
G. Sliepen
  • 69.3k
  • 3
  • 75
  • 180
Loading
added 826 characters in body
Source Link
G. Sliepen
  • 69.3k
  • 3
  • 75
  • 180
Loading
Source Link
G. Sliepen
  • 69.3k
  • 3
  • 75
  • 180
Loading