Add templated version of DEFINE_TYPE macros

[kunaltyagi]

Picking up from #2532

• Add macros for custom types in DEFINE_TYPE_{NON_,}INTRUSIVE
• Add macros for template version of DEFINE_TYPE_{NON_,}INTRUSIVE
• Refactor and add tests for templated version of DEFINE_TYPE macros

make amalgamate was not found, so the change was performed manually

---

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 file single_include/nlohmann/json.hpp. The whole process is described here.

[coveralls]

Coverage remained the same at 100.0% when pulling 4cb97ca on kunaltyagi:ordered_json into 954b10a on nlohmann:develop.

[kunaltyagi]

In the macros added, it's impossible to follow macro argument should be enclosed in parentheses [bugprone-macro-parentheses,-warnings-as-errors] since the macro arguments are the class name and the visibility type. Thoughts?

// sample code
#define start_class(A, type) class A { type:
// these wouldn't compile
#define start_class2(A, type) class (A) { type:
#define start_class3(A, type) class A { (type):

[nlohmann]

Flex check and fix the issue found in the CI:

• https://github.com/nlohmann/json/pull/2843/checks?check_run_id=2931115424
• https://github.com/nlohmann/json/pull/2843/checks?check_run_id=2931115844

[kunaltyagi]

Hopefully fixed the first. Regarding the second error, please find my analysis in the comment above yours (#2843 (comment))

[nlohmann]

There is some issue with the CI - the job ci_test_clang_sanitizer seems to fail also on develop. I'll have a look. Sorry for the inconvenience.

[nlohmann]

Can you please update to the latest develop branch? The CI issues should be fixed then. Sorry for the inconvenience.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[gregmarr]

@kunaltyagi Can you update to the latest develop branch?

[kunaltyagi]

Rebased, and also refactored the code to make the macros a bit less offensive

[kunaltyagi]

Almost all tests are passing. Current issue:

C:\projects\json\tests\src\unit-udt_macro.cpp(413): error C2220: warning treated as error - no 'object' file generated [C:\projects\json\tests\test-udt_macro_cpp11.vcxproj]
C:\projects\json\tests\src\unit-udt_macro.cpp(413): warning C4503: 'doctest::detail::instantiationHelper': decorated name length exceeded, name was truncated [C:\projects\json\tests\test-udt_macro_cpp11.vcxproj]

• Line 413 is the last line of the file
• I'm not able to experiment with windows because I have never developed on that platform
• C2220 can be ignore (essentially -Werror)
• C4503 explanation is found here

If someone can identify the source, I can make a workaround.

[falbrechtskirchinger]

It seems safe to just add a suppression for 4503.

Edit: I'm leaning towards disabling it globally in tests/CMakeLists.txt:45:

    #       Disable warning C4503: ...: decorated name length exceeded, name was truncated
    #       Disable warning C4566: character represented by universal-character-name '\uFF01'
    #                              cannot be represented in the current code page (1252)
    #       Disable warning C4996: 'nlohmann::basic_json<...>::operator <<': was declared deprecated
    $<$<CXX_COMPILER_ID:MSVC>:/W4 /wd4503 /wd4566 /wd4996>

[kunaltyagi]

All checks pass. PTAL

[kunaltyagi]

Are there any more blockers? 😅

[falbrechtskirchinger]

I don't know how much sense it makes to test the different formats here. To the extent that there are roundtrip differences, these are best tested elsewhere.
You're also missing BJData, and when the BON8 PR is merged, this is likely to be missed.

[kunaltyagi]

This is existing code, which I modified (parameterized on different types) to support different underlying implementations for json: ordered and unordered map

I'd prefer to tackle the missing BJData and BON8 in a separate PR. Thoughts?

[falbrechtskirchinger]

This is existing code, which I modified

Right. You don't need to do anything else here as far as I'm concerned.

@nlohmann @gregmarr Am I missing something? There are differences in formats (e.g., NaN serializing as null) that will cause problems when using these macros (which also might be worth mentioning in the documentation until fixed) but that's not what's being tested here. So what's the point?
Should we remove these tests (later, in a separate PR)?

[gregmarr]

Here's the PR where they were added: #2287

[falbrechtskirchinger]

I just find the choice of roundtripping bizarre. It's already tested extensively for each format and unless you're testing the corner cases (which the macros don't handle anyway), there's no reason to do so here. Let's just do something to pad the unit test! 🤷‍♂️

[gregmarr]

The referenced PR was created due to a bug where if you had 10 elements in the object, it would skip processing element 9 or just fail to compile due to an error in some hand-written code, so I think this is testing that it is including all of the named elements. #2267

[falbrechtskirchinger]

Still, why test different formats? Ignoring format-specific exceptions, a roundtrip doesn't change the basic_json.

SECTION("alphabet")
{
    json_t j = obj1;
    T obj2;
    j.get_to(obj2);
    CHECK((obj1 == obj2));
}

Is this not sufficient?

[gregmarr]

Could be, don't know for sure. @pfeatherstone did you have a particular reason for not just doing one format here?

[nlohmann]

@kunaltyagi It would be great to have this feature in the library. Do you have time to work on the remaining issues or should someone take over?

[kunaltyagi]

I think the sole unaddressed comment explicitly states that I don't need to take any action. What issues are remaining?

[nlohmann]

Oh I'm sorry - this was meant for @pfeatherstone...

[kunaltyagi]

Seems to be not available.

Btw, can we change the label on the PR?

[falbrechtskirchinger]

Is there a reason we can't make *_T the default? It wouldn't break existing code and is more sensible anyway.

Instead, I think we should (in a separate PR, of course) explore refactoring and renaming some of the implementation macros (i.e., NLOHMANN_DEFINE_TYPE_WITH_DEFAULT_IMPL) to give users the tools for creating more elaborate macros themselves, without having to delve too deeply into Preprocessor madness.

[kunaltyagi]

This PR was a pain to rebase to master. Is it possible to move it ahead?

[kunaltyagi]

Is there a reason we can't make *_T the default

Those are templated versions. They would behave a bit differently with overloading than the current versions. The existing code in the repo wouldn't be broken, but the user experience might be affected due to how templates participate in overload resolution (wrt implicit conversions), which is surprising when compared to non-overloaded versions

[falbrechtskirchinger]

Is there a reason we can't make *_T the default

Those are templated versions. They would behave a bit differently with overloading than the current versions. The existing code in the repo wouldn't be broken, but the user experience might be affected due to how templates participate in overload resolution (wrt implicit conversions), which is surprising when compared to non-overloaded versions

Non-template functions are preferred over template functions during overload resolution – true. I did not consider that. Still, what are the practical, real-world implications in this particular context? When I have some time later, I'll try to construct a contrived example to trigger different behavior, but If I even manage to do so, I wouldn't expect to find this in the wild. Maybe GitHub Code Search can help.
Also, note that all internal conversions are template functions, and if we catered to these concerns, we would never be able to add another conversion or risk interfering with existing code.

A separate issue came to mind: BasicJsonType should be constrained with nlohmann::detail::enable_if_t<typename nlohmann::detail::is_basic_json<BasicJsonType>::type, int> = 0.

[falbrechtskirchinger]

This PR was a pain to rebase to master. Is it possible to move it ahead?

The open issues are:

• I believe there's no need for separate macros. The template versions should just replace the existing ones. IMHO, the impacts of such a change are far more benign than adding a new conversion (which was done in recent history, and pending PRs would do so again).
• Depending on whether new macros are added or the existing ones are replaced, the macro documentation needs to be updated.
• Should BasicJsonType be SFINAE-constrained on to_json/from_json or is that redundant because it's already constrained earlier in the call chain?
• The unit tests for other serialization formats should be removed before merge because they serve no purpose. Let's at least make some effort for higher quality unit tests. I'll die on this hill if I have to. :-)

These shouldn't be too difficult to resolve and I'm holding back work on substantial changes until after this PR is merged.

[kunaltyagi]

A separate issue came to mind: BasicJsonType should be constrained with nlohmann::detail::enable_if_t<typename nlohmann::detail::is_basic_json<BasicJsonType>::type, int> = 0

Sure. I'll check if that's needed. If it is, I'll add that to the PR.

• The unit tests for other serialization formats should be removed before merge because they serve no purpose. Let's at least make some effort for higher quality unit tests. I'll die on this hill if I have to. :-)

Someone will have to get that merged in master outside the PR. I don't disagree on your stand, just that it's not nice to bundle it in this PR.

• I believe there's no need for separate macros.

cc: @nlohmann
Please make a decision so I can move ahead with the relevant changes. I'm not against adopting the PR, but it'd be better if we agree on a direction before I make the changes

[falbrechtskirchinger]

• The unit tests for other serialization formats should be removed before merge because they serve no purpose. Let's at least make some effort for higher quality unit tests. I'll die on this hill if I have to. :-)

Someone will have to get that merged in master outside the PR. I don't disagree on your stand, just that it's not nice to bundle it in this PR.

Right, I forgot that that's not something you added.

[nlohmann]

I agree fully with @falbrechtskirchinger.

[falbrechtskirchinger]

@kunaltyagi Heads up. To satisfy the new clang-tidy const-correctness check, we must add const everywhere. I expect the PR to be merged to develop soon.
Rebasing shouldn't be too complicated. Keep your changes and add const where possible. Don't worry about the mixed use of east and west const. We're switching to clang-format soon and will reformat the code anyway.

[Disservin]

Some months have passed whats the status on this? ;)

[YarikTH]

How many programmers need to support templated BasicJsonType in NLOHMANN_DEFINE_TYPE_MEOW? This macro is cursed