-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
truncation of constant value in to_cbor() #1286
Comments
(The function's documentation should be similar to this one.) |
@nlohmann Ok, so it is not dead code. Then there is a problem. If user calls |
What do you think about using the approach of https://stackoverflow.com/a/15172304/266378 and replace all the CharType convert(std::uint8_t x)
{
return *reinterpret_cast<CharType *>(&x);
} ? |
@nlohmann what about CharType != signed/regular/unsigned char? reinterpret_cast to char* and following dereference of char* for any pointer is safe by standard. But reinterpret_cast of pointer to some CharType = UserDefiendType? The cast itself is UB. {
int i;
int *ip = &i;
char *cp = reinterpret_cast<char*>(ip); // safe
*cp; // safe
struct MyUserType{};
MyUserType mut;
MyUserType *mutp = &mut;
cp = reinterpret_cast<char*>(mutp); // safe
*cp; // safe
// DANGER (undefined behaviour):
mutp = reinterpret_cast<MyUserType*>(cp); // UB
*mutp; // UB
mutp = reinterpret_cast<MyUserType*>(ip); // UB
*mutp; // UB
} So the function should be like: std::enable_if<
std::is_same<CharType, char>::value ||
std::is_same<CharType, unsigned char>::value ||
std::is_same<CharType, signed char>::value,
CharType
>::type convert(std::uint8_t x)
{
return *reinterpret_cast<CharType *>(&x);
} or restrict CharType to be only one of char, unsigned char, signed char in the class itself. |
For the input adapters, we require that |
C++03 5.2.10.7: C++03 3.10.15: So in general cast to any pointer except of char*/unsigned char* with dereferencing will cause UB. |
Hm. What do you propose? |
This seems like the solution tho: std::enable_if<
std::is_signed<CharType>::value && std::is_signed<char*>::value,
CharType
>::type convert(std::uint8_t x) nothrow
{
return *reinterpret_cast<char*>(&x);
}
std::enable_if<
std::is_signed<CharType>::value && std::is_unsigned<char*>::value,
CharType
>::type convert(std::uint8_t x) nothrow
{
using namespace std;
static_assert(sizeof(std::uint8_t) == sizeof(CharType), "size of CharType must be equal to std::uint8_t");
static_assert(is_pod<CharType>::value, "CharType must be POD");
CharType result;
memcpy(&result, &x, sizeof(x));
return result;
}
std::enable_if<
std::is_unsigned<CharType>::value,
CharType
>::type convert(std::uint8_t x) nothrow
{
return x;
} may be add some constexpr. |
I had to fiddle a bit to make it compile: template < typename C = CharType,
enable_if_t < std::is_signed<C>::value and std::is_signed<char>::value > * = nullptr >
static constexpr CharType convert(std::uint8_t x) noexcept
{
return *reinterpret_cast<char*>(&x);
}
template < typename C = CharType,
enable_if_t < std::is_signed<C>::value and std::is_unsigned<char>::value > * = nullptr >
static CharType convert(std::uint8_t x) noexcept
{
static_assert(sizeof(std::uint8_t) == sizeof(CharType), "size of CharType must be equal to std::uint8_t");
static_assert(std::is_pod<CharType>::value, "CharType must be POD");
CharType result;
std::memcpy(&result, &x, sizeof(x));
return result;
}
template<typename C = CharType,
enable_if_t<std::is_unsigned<C>::value>* = nullptr>
static constexpr CharType convert(std::uint8_t x) noexcept
{
return x;
} Is this what you mean? |
Yeah, sorry for my code snippet, was writing on the fly.
so
and
There is some issue that I do not know if you are taking in consideration in that project: bit count in byte. |
@oktonion Could you have a look at branch https://github.com/nlohmann/json/compare/feature/convert_char? For the non-8-bit-char platforms, I do not have an opinion right now. |
IMO, we should not worry about esoteric platforms that are not tested in our CI. We cannot verify that the unit tests even pass there, or even if they happen to, that we don't break it accidentally down the road. Platforms where "byte!=octet" (such that uint8_t is not present) are extremely rare and we should not pessimize our implementation in fear we might be breaking them when we don't even test them to start with. |
@jaredgrubb Good point but as it states in the README of the projects "What if JSON was part of modern C++?", so I assume that if this library aims to be part of standard lib then it should work on every platform possible (even with > 8 bits in byte?). #include <climits>
#if CHAR_BIT != 8
#error "JSON library is not implemented for this platform"
#endif would be nice in my opinion. |
@nlohmann also I want to thank you and the contributors for this excellent library. And I could say that it could be implemented in pure C++ 98 so I've already ported like 90% of codebase to old standard and it works like charm. |
Although documentation for
static void to_cbor(const basic_json& j, detail::output_adapter<char> o)
says nothing I assumed that it has some purpose but f.e. following sample:generates lots of warnings like
in class binary_writer
at lines like:
json/single_include/nlohmann/json.hpp
Line 7949 in e426219
or
json/single_include/nlohmann/json.hpp
Lines 7955 to 7958 in e426219
Basically the cause for that is
static_cast<CharType>(/*some integer value > 127*/)
withCharType = char
leads to loss of integer value (sizeof(char) is 1 byte and it is signed, so value range is [-128;127] and it can't fit integer number > 127).I don't know if it is an issue but that code smells.
The text was updated successfully, but these errors were encountered: