nlohmann / json

JSON for Modern C++
https://json.nlohmann.me
MIT License
41.3k stars 6.58k forks source link

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

Closed TheJCAB closed 6 months ago

TheJCAB commented 6 months ago

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

Please don't

KenSykes commented 6 months ago

include

should add since uint64_t is introduced.


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

gregmarr commented 6 months ago

include

should add since uint64_t is introduced.

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

TheJCAB commented 6 months ago

should add 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 commented 6 months ago

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

TheJCAB commented 6 months ago

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

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

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 commented 6 months ago

Yes, please always add the std part.

coveralls commented 6 months ago

Coverage Status

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

TheJCAB commented 6 months ago

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

nlohmann commented 6 months ago

Thanks a lot!

TheJCAB commented 6 months ago

Thanks a lot!

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