From e132622794af763dbc091aa6323f7bfb0b8a0f50 Mon Sep 17 00:00:00 2001 From: "Juan Carlos Arevalo Baeza (JCAB)" Date: Fri, 8 Dec 2023 10:33:41 -0800 Subject: [PATCH 1/4] Enhance the UDT unit test to expose the issue Add a new enum type with uint64_t as the underlying type. Use it in the overall UDT. Not strictly needed, but it helps exercise its expected usage. Create an object of this enum type with a large value (negative if cast to int64_t). Perform several checks on this object as converted to `json`, which fail without the fix. --- tests/src/unit-udt.cpp | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/tests/src/unit-udt.cpp b/tests/src/unit-udt.cpp index b138aa46d5..66e7130b1a 100644 --- a/tests/src/unit-udt.cpp +++ b/tests/src/unit-udt.cpp @@ -67,12 +67,15 @@ struct contact contact(person p, address a) : m_person(std::move(p)), m_address(std::move(a)) {} }; +enum class book_id : uint64_t; + struct contact_book { name m_book_name{}; + book_id m_book_id{}; std::vector m_contacts{}; contact_book() = default; - contact_book(name n, std::vector c) : m_book_name(std::move(n)), m_contacts(std::move(c)) {} + contact_book(name n, book_id i, std::vector c) : m_book_name(std::move(n)), m_book_id(std::move(i)), m_contacts(std::move(c)) {} }; } // namespace udt @@ -129,7 +132,7 @@ static void to_json(nlohmann::json& j, const contact& c) static void to_json(nlohmann::json& j, const contact_book& cb) { - j = json{{"name", cb.m_book_name}, {"contacts", cb.m_contacts}}; + j = json{{"name", cb.m_book_name}, {"id", cb.m_book_id}, {"contacts", cb.m_contacts}}; } // operators @@ -161,8 +164,8 @@ static bool operator==(const contact& lhs, const contact& rhs) static bool operator==(const contact_book& lhs, const contact_book& rhs) { - return std::tie(lhs.m_book_name, lhs.m_contacts) == - std::tie(rhs.m_book_name, rhs.m_contacts); + return std::tie(lhs.m_book_name, lhs.m_book_id, lhs.m_contacts) == + std::tie(rhs.m_book_name, rhs.m_book_id, rhs.m_contacts); } } // namespace udt @@ -219,6 +222,7 @@ static void from_json(const nlohmann::json& j, contact& c) static void from_json(const nlohmann::json& j, contact_book& cb) { cb.m_book_name = j["name"].get(); + cb.m_book_id = j["id"].get(); cb.m_contacts = j["contacts"].get>(); } } // namespace udt @@ -237,7 +241,8 @@ TEST_CASE("basic usage" * doctest::test_suite("udt")) const udt::person senior_programmer{{42}, {"王芳"}, udt::country::china}; const udt::address addr{"Paris"}; const udt::contact cpp_programmer{sfinae_addict, addr}; - const udt::contact_book book{{"C++"}, {cpp_programmer, {senior_programmer, addr}}}; + const udt::book_id large_id{udt::book_id(uint64_t(1) << 63)}; // verify large unsigned enums are handled correctly + const udt::contact_book book{{"C++"}, {udt::book_id(42u)}, {cpp_programmer, {senior_programmer, addr}}}; SECTION("conversion to json via free-functions") { @@ -248,21 +253,26 @@ TEST_CASE("basic usage" * doctest::test_suite("udt")) CHECK(json("Paris") == json(addr)); CHECK(json(cpp_programmer) == R"({"person" : {"age":23, "name":"theo", "country":"France"}, "address":"Paris"})"_json); + CHECK(json(large_id) == json(uint64_t(1) << 63)); + CHECK(json(large_id) > 0u); + CHECK(to_string(json(large_id)) == "9223372036854775808"); + CHECK(json(large_id).is_number_unsigned()); CHECK( json(book) == - R"({"name":"C++", "contacts" : [{"person" : {"age":23, "name":"theo", "country":"France"}, "address":"Paris"}, {"person" : {"age":42, "country":"中华人民共和国", "name":"王芳"}, "address":"Paris"}]})"_json); + R"({"name":"C++", "id":42, "contacts" : [{"person" : {"age":23, "name":"theo", "country":"France"}, "address":"Paris"}, {"person" : {"age":42, "country":"中华人民共和国", "name":"王芳"}, "address":"Paris"}]})"_json); } SECTION("conversion from json via free-functions") { const auto big_json = - R"({"name":"C++", "contacts" : [{"person" : {"age":23, "name":"theo", "country":"France"}, "address":"Paris"}, {"person" : {"age":42, "country":"中华人民共和国", "name":"王芳"}, "address":"Paris"}]})"_json; + R"({"name":"C++", "id":42, "contacts" : [{"person" : {"age":23, "name":"theo", "country":"France"}, "address":"Paris"}, {"person" : {"age":42, "country":"中华人民共和国", "name":"王芳"}, "address":"Paris"}]})"_json; SECTION("via explicit calls to get") { const auto parsed_book = big_json.get(); const auto book_name = big_json["name"].get(); + const auto book_id = big_json["id"].get(); const auto contacts = big_json["contacts"].get>(); const auto contact_json = big_json["contacts"].at(0); @@ -282,6 +292,7 @@ TEST_CASE("basic usage" * doctest::test_suite("udt")) CHECK(contact == cpp_programmer); CHECK(contacts == book.m_contacts); CHECK(book_name == udt::name{"C++"}); + CHECK(book_id == book.m_book_id); CHECK(book == parsed_book); } @@ -303,6 +314,7 @@ TEST_CASE("basic usage" * doctest::test_suite("udt")) { const udt::contact_book parsed_book = big_json; const udt::name book_name = big_json["name"]; + const udt::book_id book_id = big_json["id"]; const std::vector contacts = big_json["contacts"]; const auto contact_json = big_json["contacts"].at(0); const udt::contact contact = contact_json; @@ -320,6 +332,7 @@ TEST_CASE("basic usage" * doctest::test_suite("udt")) CHECK(contact == cpp_programmer); CHECK(contacts == book.m_contacts); CHECK(book_name == udt::name{"C++"}); + CHECK(book_id == udt::book_id(42u)); CHECK(book == parsed_book); } #endif From 2eb72beedb8ff6526978de38c8b286b887f78b21 Mon Sep 17 00:00:00 2001 From: "Juan Carlos Arevalo Baeza (JCAB)" Date: Fri, 8 Dec 2023 10:35:09 -0800 Subject: [PATCH 2/4] Fix the issue in the relevant `to_json` overload. Select the correct json type depending on the signedness of the enum's underlying type. This fixes the new checks in the unit test. --- include/nlohmann/detail/conversions/to_json.hpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/include/nlohmann/detail/conversions/to_json.hpp b/include/nlohmann/detail/conversions/to_json.hpp index e39b7797dd..8a8610821d 100644 --- a/include/nlohmann/detail/conversions/to_json.hpp +++ b/include/nlohmann/detail/conversions/to_json.hpp @@ -320,7 +320,14 @@ template::type; - external_constructor::construct(j, static_cast(e)); + if (std::is_unsigned::value) + { + external_constructor::construct(j, static_cast(e)); + } + else + { + external_constructor::construct(j, static_cast(e)); + } } #endif // JSON_DISABLE_ENUM_SERIALIZATION From 1402ca486594e95ae152617ecfed4770ae90603f Mon Sep 17 00:00:00 2001 From: "Juan Carlos Arevalo Baeza (JCAB)" Date: Fri, 8 Dec 2023 14:02:53 -0800 Subject: [PATCH 3/4] Add the fix to the single_include I ran `make pretty` but that modified 20 files, performing a significant amount of indentation changes, none of them related to my change. I ran `make amalgamate`, but that did nothing. Apparently, the make rule won't run if the single_include files have already been updated by `make pretty`. I forced `make amalgamate` to do the work by touching the file with the fix. I then decided to keep just the minimal needed change: the addition of the fix to the single_include file. I just am not conversant enough in Linux to know whether I installed astyle correctly (had to clone the source from a beta branch and build, in order to get support for `--squeeze-lines`). --- single_include/nlohmann/json.hpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp index 8b72ea6539..1d479ceb17 100644 --- a/single_include/nlohmann/json.hpp +++ b/single_include/nlohmann/json.hpp @@ -5697,7 +5697,14 @@ template::type; - external_constructor::construct(j, static_cast(e)); + if (std::is_unsigned::value) + { + external_constructor::construct(j, static_cast(e)); + } + else + { + external_constructor::construct(j, static_cast(e)); + } } #endif // JSON_DISABLE_ENUM_SERIALIZATION From 5af4ec4f6bcedad87c4afca42f88e534befcc52c Mon Sep 17 00:00:00 2001 From: "Juan Carlos Arevalo Baeza (JCAB)" Date: Sun, 10 Dec 2023 20:00:47 -0800 Subject: [PATCH 4/4] Resolve CI errors and use qualified `std::uint64_t` The fix was relying on implicit conversions in the non-taken branch. - Ordinarily (work on a C++20 codebase) I would have used `if constexpr` here, sidestepping the issue, but that's not available on C++11 so I didn't bother. - So instead of an `if` statement, I used a compile-time constant to select the correct overload. - This is arguably better in this case, anyway. I was using function-style casts for typed constants, which I consider superior for constants, but the CI checks disagree, so changed all to `static_cast`. - For some reason, the CI checks didn't point at all of them, so I hope I caught them all myself. Built with clang14 and all unit tests pass. --- include/nlohmann/detail/conversions/to_json.hpp | 10 ++-------- single_include/nlohmann/json.hpp | 10 ++-------- tests/src/unit-udt.cpp | 12 ++++++------ 3 files changed, 10 insertions(+), 22 deletions(-) diff --git a/include/nlohmann/detail/conversions/to_json.hpp b/include/nlohmann/detail/conversions/to_json.hpp index 8a8610821d..562089c330 100644 --- a/include/nlohmann/detail/conversions/to_json.hpp +++ b/include/nlohmann/detail/conversions/to_json.hpp @@ -320,14 +320,8 @@ template::type; - if (std::is_unsigned::value) - { - external_constructor::construct(j, static_cast(e)); - } - else - { - external_constructor::construct(j, static_cast(e)); - } + static constexpr value_t integral_value_t = std::is_unsigned::value ? value_t::number_unsigned : value_t::number_integer; + external_constructor::construct(j, static_cast(e)); } #endif // JSON_DISABLE_ENUM_SERIALIZATION diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp index 1d479ceb17..b191bb9140 100644 --- a/single_include/nlohmann/json.hpp +++ b/single_include/nlohmann/json.hpp @@ -5697,14 +5697,8 @@ template::type; - if (std::is_unsigned::value) - { - external_constructor::construct(j, static_cast(e)); - } - else - { - external_constructor::construct(j, static_cast(e)); - } + static constexpr value_t integral_value_t = std::is_unsigned::value ? value_t::number_unsigned : value_t::number_integer; + external_constructor::construct(j, static_cast(e)); } #endif // JSON_DISABLE_ENUM_SERIALIZATION diff --git a/tests/src/unit-udt.cpp b/tests/src/unit-udt.cpp index 66e7130b1a..b8437def53 100644 --- a/tests/src/unit-udt.cpp +++ b/tests/src/unit-udt.cpp @@ -67,7 +67,7 @@ struct contact contact(person p, address a) : m_person(std::move(p)), m_address(std::move(a)) {} }; -enum class book_id : uint64_t; +enum class book_id : std::uint64_t; struct contact_book { @@ -75,7 +75,7 @@ struct contact_book book_id m_book_id{}; std::vector m_contacts{}; contact_book() = default; - contact_book(name n, book_id i, std::vector c) : m_book_name(std::move(n)), m_book_id(std::move(i)), m_contacts(std::move(c)) {} + contact_book(name n, book_id i, std::vector c) : m_book_name(std::move(n)), m_book_id(i), m_contacts(std::move(c)) {} }; } // namespace udt @@ -241,8 +241,8 @@ TEST_CASE("basic usage" * doctest::test_suite("udt")) const udt::person senior_programmer{{42}, {"王芳"}, udt::country::china}; const udt::address addr{"Paris"}; const udt::contact cpp_programmer{sfinae_addict, addr}; - const udt::book_id large_id{udt::book_id(uint64_t(1) << 63)}; // verify large unsigned enums are handled correctly - const udt::contact_book book{{"C++"}, {udt::book_id(42u)}, {cpp_programmer, {senior_programmer, addr}}}; + const udt::book_id large_id{static_cast(static_cast(1) << 63)}; // verify large unsigned enums are handled correctly + const udt::contact_book book{{"C++"}, static_cast(42u), {cpp_programmer, {senior_programmer, addr}}}; SECTION("conversion to json via free-functions") { @@ -253,7 +253,7 @@ TEST_CASE("basic usage" * doctest::test_suite("udt")) CHECK(json("Paris") == json(addr)); CHECK(json(cpp_programmer) == R"({"person" : {"age":23, "name":"theo", "country":"France"}, "address":"Paris"})"_json); - CHECK(json(large_id) == json(uint64_t(1) << 63)); + CHECK(json(large_id) == json(static_cast(1) << 63)); CHECK(json(large_id) > 0u); CHECK(to_string(json(large_id)) == "9223372036854775808"); CHECK(json(large_id).is_number_unsigned()); @@ -332,7 +332,7 @@ TEST_CASE("basic usage" * doctest::test_suite("udt")) CHECK(contact == cpp_programmer); CHECK(contacts == book.m_contacts); CHECK(book_name == udt::name{"C++"}); - CHECK(book_id == udt::book_id(42u)); + CHECK(book_id == static_cast(42u)); CHECK(book == parsed_book); } #endif