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

Generate template functions with NLOHMANN_DEFINE_TYPE macros #4597

Merged

Conversation

kimci86
Copy link
Contributor

@kimci86 kimci86 commented Jan 13, 2025

Changes:

  • make NLOHMANN_DEFINE_TYPE_* generate template functions. This allows to use any specialization of nlohmann::basic_json such as nlohmann::ordered_json, instead of nlohmann::json only.
  • update and extend related tests
  • update related documentation

Existing issues:

Related PRs:

Reviews on related PRs have raised the following concerns:

  • There might be a risk that generated to_json/from_json becoming a template change overload resolution is some twisted cases.
  • The template type BasicJsonType is not restricted to basic_json instances.

To be noted: NLOHMANN_JSON_SERIALIZE_ENUM already uses that same design of unrestricted BasicJsonType template parameter.
If we want to restrict it, I see two options:

  • Restructure headers so that macros can be defined after traits to be able to use is_basic_json.
    Currently include/nlohmann/detail/meta/type_traits.hpp includes include/nlohmann/detail/macro_scope.hpp so we cannot use is_basic_json in macros.
  • Do not #undef NLOHMANN_BASIC_JSON_TPL_DECLARATION and NLOHMANN_BASIC_JSON_TPL in macro_unscope.hpp so that we can use them in macros to effectively write the full basic_json template type.

What do you think?

Thank you for this awesome library!

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.

@kimci86 kimci86 requested a review from nlohmann as a code owner January 13, 2025 21:44
@kimci86 kimci86 force-pushed the template_nlohmann_define_type_macros branch from 9f84443 to b22b43a Compare January 13, 2025 21:46
@kimci86
Copy link
Contributor Author

kimci86 commented Jan 13, 2025

On second thought, I can use nlohmann::detail::is_basic_json in those macro definitions. As long as nlohmann/detail/meta/type_traits.hpp is included at some point before using the macros, it would work. It looks a little fragile to me, but maybe that is good enough?

@nlohmann
Copy link
Owner

(I'm only commenting on the CI issues right now: Clang-Tidy should be fixable by just adding some const. Coverage is currently broken and #4595 provides a fix which is not yet merged.)

@nlohmann
Copy link
Owner

The coverage job is now fixed in the develop branch. Please update.

@nlohmann nlohmann added the please rebase Please rebase your branch to origin/develop label Jan 15, 2025
@kimci86 kimci86 force-pushed the template_nlohmann_define_type_macros branch from 749b983 to 43c41f0 Compare January 15, 2025 18:06
@coveralls
Copy link

coveralls commented Jan 15, 2025

Coverage Status

coverage: 99.184%. remained the same
when pulling 67c46ee on kimci86:template_nlohmann_define_type_macros
into e72046e on nlohmann:develop.

Signed-off-by: kimci86 <kimci86@hotmail.fr>
Signed-off-by: kimci86 <kimci86@hotmail.fr>
Signed-off-by: kimci86 <kimci86@hotmail.fr>
Signed-off-by: kimci86 <kimci86@hotmail.fr>
Signed-off-by: kimci86 <kimci86@hotmail.fr>
Signed-off-by: kimci86 <kimci86@hotmail.fr>
Signed-off-by: kimci86 <kimci86@hotmail.fr>
Signed-off-by: kimci86 <kimci86@hotmail.fr>
Signed-off-by: kimci86 <kimci86@hotmail.fr>
@kimci86 kimci86 force-pushed the template_nlohmann_define_type_macros branch from 43c41f0 to 2a90012 Compare January 18, 2025 11:30
@kimci86
Copy link
Contributor Author

kimci86 commented Jan 18, 2025

Rebased and fixed conflicts.

Signed-off-by: kimci86 <kimci86@hotmail.fr>
@nlohmann nlohmann removed the please rebase Please rebase your branch to origin/develop label Jan 18, 2025
Signed-off-by: kimci86 <kimci86@hotmail.fr>
@nlohmann
Copy link
Owner

On second thought, I can use nlohmann::detail::is_basic_json in those macro definitions. As long as nlohmann/detail/meta/type_traits.hpp is included at some point before using the macros, it would work. It looks a little fragile to me, but maybe that is good enough?

Yes, I think using nlohmann::detail::is_basic_json to constrain the type is a good idea. Who knows which other types may accidentally be caught my the macros otherwise...

Signed-off-by: kimci86 <kimci86@hotmail.fr>
@kimci86 kimci86 force-pushed the template_nlohmann_define_type_macros branch from 13d2d07 to a6d937c Compare January 18, 2025 15:59
@kimci86
Copy link
Contributor Author

kimci86 commented Jan 18, 2025

Added type constraint via SFINAE. Not sure it is relevant to show this kind of ugly type in documentation too, but at least it describes accurately what happens.

@nlohmann
Copy link
Owner

No, don't show this in the documentation.

@kimci86 kimci86 force-pushed the template_nlohmann_define_type_macros branch from a6d937c to 5d3d8fa Compare January 18, 2025 16:46
@kimci86
Copy link
Contributor Author

kimci86 commented Jan 18, 2025

Ok I dropped a6d937c then.

@nlohmann

This comment was marked as resolved.

Signed-off-by: kimci86 <kimci86@hotmail.fr>
@kimci86
Copy link
Contributor Author

kimci86 commented Jan 18, 2025

Can you please add a brief documentation for the template parameter?

Sorry I am not sure to understand where I should add such documentation.
It is not a template parameter of the macros themselves, but of the generated function.

@nlohmann
Copy link
Owner

Can you please add a brief documentation for the template parameter?

Sorry I am not sure to understand where I should add such documentation. It is not a template parameter of the macros themselves, but of the generated function.

You are right - the macros have no template parameter. My bad.

Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@nlohmann nlohmann added this to the Release 3.11.4 milestone Jan 18, 2025
@nlohmann nlohmann merged commit 8a882f3 into nlohmann:develop Jan 18, 2025
130 checks passed
@nlohmann
Copy link
Owner

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants