boostorg / endian

Boost Endian library
45 stars 49 forks source link

Conversion support for additional types #10

Closed Beman closed 5 years ago

Beman commented 9 years ago

conversion.hpp should support char, wchar_t, char16_t, and char32_t.

norbertwenzel commented 7 years ago

Despite these types not having any explicit overloads of endian_reverse() it seems to work (ie. compile an native_to_big()) at least on my machine using -std=c++11 -pedantic and Boost 1.63:

$ g++ --version
g++ (Ubuntu 5.4.1-2ubuntu1~14.04) 5.4.1 20160904
tomsmeding commented 5 years ago

With boost 1.69 (develop has no relevant changes as far as I can see), endian_reverse() fails when passed an argument with type char. To be precise, on my systems, the following assert fails:

char c = 'a';
assert(boost::endian::endian_reverse(c) == c);

The reason is that (on my systems), char, signed char = int8_t and unsigned char = uint8_t are three different types. Since endian_reverse has no char overload, the int8_t overload is not an exact match, and apparently gcc prefers the int = int32_t overload in that case. endian_reverse then incorrectly returns zero for obvious reasons.

Adding an overload char endian_reverse(char x) { return x; } in conversion.hpp fixes the problem for me. Am I right, and would this be the correct fix?

pdimov commented 5 years ago

What are your systems?

tomsmeding commented 5 years ago

Arch linux (boost 1.69.0) on a recent Intel Core i7, and debian stable (boost 1.62.0) on another recent Intel Core i7. On both machines, char is signed, but unequal to signed char. By the way, thanks for looking into this!

pdimov commented 5 years ago

On one hand, only providing overloads for the exact-width types seems to be a deliberate design decision:

https://github.com/boostorg/endian/blob/3c6c06616d9684601a4b83577db25302d90d1570/include/boost/endian/conversion.hpp#L52-L55

On the other, calling endian_reverse already works for most ordinary types such as short and int, and for char and char16_t, it produces the wrong answer.

The right course of action seems to be to make it work correctly for all built-in integral types. Probably after 1.70 is released though.

Jeff-G commented 5 years ago

This library is also missing the unsigned long type. The following error:

error C2668: 'boost::endian::endian_reverse': ambiguous call to overloaded function

Is generated by the following code:

unsigned long val = 5;
boost::endian::native_to_big_inplace(val);

This is really annoying. Now I have to write a bunch template specialization wrappers with nasty static_assert statements because the implementation of boost::endian is incomplete.

template<typename T>
void to_big(T &val) {
    boost::endian::native_to_big_inplace(val);
}

template<>
void to_big(unsigned long &val) {
    static_assert(sizeof(unsigned long) == sizeof(uint32_t));
    // I can't use the inplace implementation, since casting from unsigned long *
    // to uint32_t* results in undefined behavior.
    val = boost::endian::native_to_big_inplace<uint32_t>(val);
}
pdimov commented 5 years ago

I intend to fix this for the next release, see https://github.com/boostorg/endian/blob/7c35802f6a4b37d906cbe4f0a1c2795fa764782e/test/endian_reverse_test.cpp#L142

pdimov commented 5 years ago

Should be fixed in the current develop.