Skip to content
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

Properly convert constants to CharType #1315

Merged
merged 4 commits into from
Oct 27, 2018
Merged

Conversation

nlohmann
Copy link
Owner

Fixes #1286.

@nlohmann nlohmann added the aspect: binary formats BSON, CBOR, MessagePack, UBJSON label Oct 23, 2018
@jaredgrubb
Copy link
Contributor

+1
I really don't like that code, but necessary evil I suppose :)

@jaredgrubb
Copy link
Contributor

Actually: do you think there should be some comments in this code to just link to this Issue or the SO post that explains why it's necessary? I suppose you could get some of that via GitHub, but that might get buried? very minor thing you can ignore.

@oktonion
Copy link

@nlohmann That's it. Except if you are using it with signed chars (like 'l') then for the sake of micro optimization we could add some overload like

    template < typename C = CharType,
               enable_if_t < std::is_signed<C>::value and std::is_signed<char>::value > * = nullptr >
    static constexpr CharType to_char_type(char x) noexcept
    {
        return x;
    }

Idk if it is so necessary tho.

@jaredgrubb I think that with some bit shift magic this could be done in less obvious but more compact style. But I prefer straightforward memcpy to be on the safe side.

@nlohmann
Copy link
Owner Author

@oktonion The code you propose has the signature CharType to_char_type(char x), whereas all the other functions have CharType to_char_type(std::uint8_t x). Did I miss anything?

@coveralls
Copy link

coveralls commented Oct 24, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling c2e1757 on feature/convert_char into 9294e25 on develop.

@oktonion
Copy link

@nlohmann my bad, there is a working example:

// Example program (I removed constexpr for the sake of std::cout calls)
#include <iostream>
#include <string>
#include <cstring>
    
    template<bool B, typename T = void>
    using enable_if_t = typename std::enable_if<B, T>::type;
    
    template < typename C = CharType,
               enable_if_t < std::is_signed<C>::value and std::is_signed<char>::value > * = nullptr >
    static CharType to_char_type(std::uint8_t x) noexcept
    {
        std::cout << "1" << std::endl;
        
        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 to_char_type(std::uint8_t x) noexcept
    {
        std::cout << "2" << std::endl;
        
        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 CharType to_char_type(std::uint8_t x) noexcept
    {
        std::cout << "3" << std::endl;
        
        return x;
    }
    
    template < typename InputCharType,
                typename C = CharType,
               enable_if_t < 
               std::is_signed<C>::value and 
               std::is_signed<char>::value and 
               std::is_same<char, typename std::remove_cv<InputCharType>::type>::value 
               > * = nullptr >
    static CharType to_char_type(InputCharType x) noexcept
    {
        std::cout << "4" << std::endl;
        
        return x;
    }

int main()
{
    to_char_type(0x08);
    to_char_type(-23);
    to_char_type(254);
    to_char_type('l');
}

so for typedef unsigned char CharType; the output is

3
3
3
3

for typedef char CharType; the output is (note that '1' in 4th call is replaced with '4' overload)

1
1
1
4

the '4' is faster than '1' and more clear for compiler optimizer. It's micro optimization though.

@nlohmann nlohmann self-assigned this Oct 25, 2018
@nlohmann nlohmann added this to the Release 3.3.1 milestone Oct 25, 2018
@jaredgrubb
Copy link
Contributor

If you really want to stress-test your implementation, make sure to try all three char, unsigned char, and signed char (remember that these three types are distinct types in C++, they are not aliases [eg, like how "signed int" and "int" are the same]); you could also force your compiler to flip the signedness of char (eg, -funsigned-char and -fsigned-char for GCC/clang).

@nlohmann nlohmann merged commit 1308ea0 into develop Oct 27, 2018
@nlohmann
Copy link
Owner Author

Thanks everyone!

@nlohmann nlohmann deleted the feature/convert_char branch October 28, 2018 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants