Fix `to_json` for enums when the enum has an unsigned underlying type. #4237

TheJCAB · 2023-12-08T22:24:37Z

This fixes issue #4236.

The fix is simple. In the relevant to_json overload, we just need to select the correct json type depending on the signedness of the enum's underlying type.

The unit-udt test is also updated to cover this corner case in a way that fails without the fix.

Note that amalgamating the source was a bit of a story for me:

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. This makes sense but it can be confusing.
I forced make amalgamate to do the work by touching the file that had 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). I decided to just let more accustomed contributors deal with this if needed.

Pull request checklist

Read the Contribution Guidelines for detailed information.

Changes are described in the pull request, or an existing issue is referenced.
The test suite compiles and runs without error.
Code coverage is 100%. Test cases can be added by editing the test suite.
The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header files single_include/nlohmann/json.hpp and single_include/nlohmann/json_fwd.hpp. The whole process is described here.

Please don't

The C++11 support varies between different compilers and versions. Please note the list of supported compilers. Some compilers like GCC 4.7 (and earlier), Clang 3.3 (and earlier), or Microsoft Visual Studio 13.0 and earlier are known not to work due to missing or incomplete C++11 support. Please refrain from proposing changes that work around these compiler's limitations with #ifdefs or other means.
Specifically, I am aware of compilation problems with Microsoft Visual Studio (there even is an issue label for this kind of bug). I understand that even in 2016, complete C++11 support isn't there yet. But please also understand that I do not want to drop features or uglify the code just to make Microsoft's sub-standard compiler happy. The past has shown that there are ways to express the functionality such that the code compiles with the most recent MSVC - unfortunately, this is not the main objective of the project.
Please refrain from proposing changes that would break JSON conformance. If you propose a conformant extension of JSON to be supported by the library, please motivate this extension.
Please do not open pull requests that address multiple issues.

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.

Select the correct json type depending on the signedness of the enum's underlying type. This fixes the new checks in the unit test.

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`).

KenSykes · 2023-12-08T22:27:54Z

#include

should add <stdint.h> since uint64_t is introduced.

Refers to: tests/src/unit-udt.cpp:21 in 1402ca4. [](commit_id = 1402ca4, deletion_comment = False)

gregmarr · 2023-12-08T22:36:14Z

#include

should add <stdint.h> since uint64_t is introduced.

The <cstdint> header is already being included by json.hpp, so this should probably be std::uint64_t.

TheJCAB · 2023-12-08T22:36:45Z

should add <stdint.h> since uint64_t is introduced.

Thanx! Looking through the codebase I saw this is typically not done. Also, I saw that std::uint64_t and plain uint64_t are both used in the tests indistinctly, even within the same file. So I decided to keep the change simple and minimal.

gregmarr · 2023-12-08T22:55:53Z

If bare uint64_t is already being used, then you should be fine.

TheJCAB · 2023-12-08T23:19:49Z

@gregmarr: TBH I'm considering it. It looks like <stdint.h> is being included via the Hedley library.

Trivia (which we have had to contend with recently 😊):

The standard dictates that provides std::blah_t types whereas <stdint.h> provides blah_t types.
It also says that both headers can optionally choose to also provide the other form.
When building with the Microsoft Visual C++ compiler, both headers are equivalent: they provide both forms.
When building with Clang (not sure about GCC) it adheres to the minimal specification so the optional forms are not provided by either header.

As for this json library: it includes both (via Hedley) so it provides both. Arguably, std::blah_t is more strictly correct for C++, so I'll make this change if other changes are required, or if the owner asks for it.

Thanx!

nlohmann · 2023-12-09T06:43:58Z

Yes, please always add the std part.

coveralls · 2023-12-09T07:06:07Z

coverage: 100.0%. remained the same
when pulling 5af4ec4 on TheJCAB:issue4236
into 3780b41 on nlohmann:develop.

nlohmann

Some minor things. Please have a look at the failing CI jobs.

tests/src/unit-udt.cpp

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.

TheJCAB · 2023-12-11T04:03:47Z

@nlohmann thanx for the review. I hope I got everything proper this time.

nlohmann

Looks good to me.

nlohmann · 2023-12-14T08:26:15Z

Thanks a lot!

TheJCAB · 2023-12-14T15:32:09Z

Thanks a lot!

My pleasure. This library is a neat piece of work, thanks!

TheJCAB added 3 commits December 8, 2023 10:33

Fix the issue in the relevant to_json overload.

2eb72be

Select the correct json type depending on the signedness of the enum's underlying type. This fixes the new checks in the unit test.

TheJCAB requested a review from nlohmann as a code owner December 8, 2023 22:24

github-actions bot added M tests labels Dec 8, 2023

nlohmann requested changes Dec 9, 2023

View reviewed changes

tests/src/unit-udt.cpp Outdated Show resolved Hide resolved

tests/src/unit-udt.cpp Outdated Show resolved Hide resolved

TheJCAB requested a review from nlohmann December 13, 2023 19:56

nlohmann linked an issue Dec 14, 2023 that may be closed by this pull request

to_json is erroneously converting enums with underlying unsigned types to signed numbers #4236

Closed

2 tasks

nlohmann added this to the Release 3.11.4 milestone Dec 14, 2023

nlohmann approved these changes Dec 14, 2023

View reviewed changes

nlohmann merged commit a259ecc into nlohmann:develop Dec 14, 2023
111 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `to_json` for enums when the enum has an unsigned underlying type. #4237

Fix `to_json` for enums when the enum has an unsigned underlying type. #4237

TheJCAB commented Dec 8, 2023

KenSykes commented Dec 8, 2023

gregmarr commented Dec 8, 2023

TheJCAB commented Dec 8, 2023

gregmarr commented Dec 8, 2023

TheJCAB commented Dec 8, 2023

nlohmann commented Dec 9, 2023

coveralls commented Dec 9, 2023 •

edited

Loading

nlohmann left a comment

TheJCAB commented Dec 11, 2023

nlohmann left a comment

nlohmann commented Dec 14, 2023

TheJCAB commented Dec 14, 2023

Fix to_json for enums when the enum has an unsigned underlying type. #4237

Fix to_json for enums when the enum has an unsigned underlying type. #4237

Conversation

TheJCAB commented Dec 8, 2023

Pull request checklist

Please don't

KenSykes commented Dec 8, 2023

gregmarr commented Dec 8, 2023

TheJCAB commented Dec 8, 2023

gregmarr commented Dec 8, 2023

TheJCAB commented Dec 8, 2023

nlohmann commented Dec 9, 2023

coveralls commented Dec 9, 2023 • edited Loading

nlohmann left a comment

Choose a reason for hiding this comment

TheJCAB commented Dec 11, 2023

nlohmann left a comment

Choose a reason for hiding this comment

nlohmann commented Dec 14, 2023

TheJCAB commented Dec 14, 2023

Fix `to_json` for enums when the enum has an unsigned underlying type. #4237

Fix `to_json` for enums when the enum has an unsigned underlying type. #4237

coveralls commented Dec 9, 2023 •

edited

Loading