Add versioned, ABI-tagged inline namespace and namespace macros #3590

falbrechtskirchinger · 2022-07-18T20:50:30Z

Add a versioned and ABI-tagged inline namespace to prevent ABI issues when linking code using multiple library versions.
Add namespace scope and name macros, i.e., NLOHMANN_JSON_NAMESPACE_BEGIN, NLOHMANN_JSON_NAMESPACE_END, NLOHMANN_JSON_NAMESPACE.

Fixes #3588.
Fixes #3360.

To Do:

Update documentation.
- Add macro pages.
- Mixing JSON_DIAGNOSTICS on and off is now supported.
- Mention namespace scope macros wherever the user is supposed to place code into the nlohmann namespace (adl_serializer, are there more?).
Maybe add a special unit test. (Would have to add 3.10.5 release single header now and pre-3.11.0 develop header later for complete coverage.)
Check namespace use in unit tests.

coveralls · 2022-07-18T21:38:54Z

Coverage remained the same at 100.0% when pulling 0ac4c8a on falbrechtskirchinger:inline-ns into d1d79b9 on nlohmann:develop.

gregmarr · 2022-07-18T22:02:30Z

Can these be updated by https://github.com/nlohmann/json_release_scripts/blob/main/scripts/bump_version/bump_version.py?

falbrechtskirchinger · 2022-07-19T04:06:32Z

Yes. I already have a modified version, although that script needs several updates because of other changes.

falbrechtskirchinger · 2022-07-19T07:16:24Z

Now that I've switched to macros, updating the version via bump_version.py is even easier.

Overall, the solution isn't all that pretty - with or without macros. Also, json_fwd.hpp does now include macro_scope.hpp. I think it's time to split macro_scope.hpp and move the more complicated things (NLOHMANN_JSON_SERIALIZE_ENUM, NLOHMANN_CAN_CALL_STD_FUNC_IMPL, ...) into a separate file.

I'm strongly leaning toward adding special unit tests for these changes (see To Do in the description).

falbrechtskirchinger · 2022-07-19T11:32:31Z

Note to self: 196f0acc7e39de04b730a5f33f038da9af2847f2 "Replace uses of nlohmann namespace with macro"

docs/mkdocs/docs/api/macros/index.md

docs/mkdocs/docs/api/macros/json_diagnostics.md

include/nlohmann/detail/macro_scope.hpp

include/nlohmann/detail/meta/call_std/begin.hpp

tests/abi/use_this.cpp

gregmarr

Does this eliminate the need for this check, or does it simply mean that we only need to check for versions prior to 3.11.0 being included?

https://github.com/nlohmann/json/pull/3590/files?diff=split&w=1#diff-b56a00981d8f3b87e3ce49a7eb27d36f4586d9c54c3fb628a88cfc000aa5fed4R42-R48

Could we have a test of an object created with JSON_DIAGNOSTICS 1 being passed to a function defined in a file with JSON_DIAGNOSTICS 0? I'm hoping that it fails with a reasonable error message, but it could also copy, like ordered_json to json.

docs/mkdocs/docs/api/macros/json_diagnostics.md

docs/mkdocs/docs/api/macros/nlohmann_json_namespace.md

include/nlohmann/detail/conversions/to_json.hpp

gregmarr · 2022-07-19T14:51:58Z

Overall, the solution isn't all that pretty - with or without macros. Also, json_fwd.hpp does now include macro_scope.hpp. I think it's time to split macro_scope.hpp and move the more complicated things (NLOHMANN_JSON_SERIALIZE_ENUM, NLOHMANN_CAN_CALL_STD_FUNC_IMPL, ...) into a separate file.

Yes, I could see splitting this up into something like "macros which affect the definition of the basic_json type and need to be included in json_fwd" and "macros which are used in the implementation or by users".

falbrechtskirchinger · 2022-07-19T15:42:37Z

Overall, the solution isn't all that pretty - with or without macros. Also, json_fwd.hpp does now include macro_scope.hpp. I think it's time to split macro_scope.hpp and move the more complicated things (NLOHMANN_JSON_SERIALIZE_ENUM, NLOHMANN_CAN_CALL_STD_FUNC_IMPL, ...) into a separate file.

Yes, I could see splitting this up into something like "macros which affect the definition of the basic_json type and need to be included in json_fwd" and "macros which are used in the implementation or by users".

I've added abi_macros.hpp and included it where appropriate.

falbrechtskirchinger · 2022-07-19T15:50:04Z

Does this eliminate the need for this check, or does it simply mean that we only need to check for versions prior to 3.11.0 being included?

https://github.com/nlohmann/json/pull/3590/files?diff=split&w=1#diff-b56a00981d8f3b87e3ce49a7eb27d36f4586d9c54c3fb628a88cfc000aa5fed4R42-R48

I think it is still useful. Including multiple versions in one translation unit can potentially be handled by ordering the includes carefully and creating using aliases with the current value of NLOHMANN_JSON_NAMESPACE, but should be discouraged.
Come to think of it, I meant to not blindly re-define NLOHMANN_JSON_NAMESPACE and did in fact document, that it's user-definable.

Could we have a test of an object created with JSON_DIAGNOSTICS 1 being passed to a function defined in a file with JSON_DIAGNOSTICS 0? I'm hoping that it fails with a reasonable error message, but it could also copy, like ordered_json to json.

Will do. If it converts, that would be a problematic outcome.

gregmarr · 2022-07-19T17:19:21Z

I think it is still useful. Including multiple versions in one translation unit can potentially be handled by ordering the includes carefully and creating using aliases with the current value of NLOHMANN_JSON_NAMESPACE, but should be discouraged.

Oh, it's just a warning. I was thinking it was an error.

falbrechtskirchinger · 2022-07-19T18:12:41Z

Alright, I've spent some time building a test that passes a nlohmann::json_v3_10_5_diag::json (i.e., diagnostics on) to a function accepting nlohmann::json_v3_10_5::json (i.e., diagnostics off). Worse than expected, the result is effectively a reinterpret_cast. :-(
This happens because the return type in the header is written as nlohmann::json (i.e., without the inline namespace) and is interpreted as whatever that type expands to based on the header included into the current translation unit.

Inline namespaces only solve the use case where multiple components use the JSON library internally but don't expose those objects to the user. I still think interoperability is possible with very carefully written glue code. That's what I'll try next (using typeid() to verify correctness).

The documentation needs some more work in light of this.

nlohmann · 2022-07-20T09:29:54Z

I was not aware of inline namespaces. Thanks for adding this. I found https://www.foonathan.net/2018/11/inline-namespaces/ pretty useful.

tests/abi/CMakeLists.txt

falbrechtskirchinger · 2022-07-20T09:52:22Z

I was not aware of inline namespaces. Thanks for adding this. I found https://www.foonathan.net/2018/11/inline-namespaces/ pretty useful.

This reminds me that I wanted to rethink the documentation changes around arbitrary type conversion.

Add a versioned inline namespace to prevent ABI issues when linking code using multiple library versions.

Add _diag suffix to inline namespace if JSON_DIAGNOSTICS is enabled, and _ldvcmp suffix if JSON_USE_LEGACY_DISCARDED_VALUE_COMPARISON is enabled.

falbrechtskirchinger · 2022-07-29T12:36:29Z

@nlohmann I'm done here. Ready for review.

include/nlohmann/detail/abi_macros.hpp

nlohmann

Looks good to me.

Since nlohmann/json#3590, the basic_json class and the json using-declaration are located in a "versioned, ABI-tagged inline namespace". This makes it impossible to forward declare the type in REveElement.hxx. Instead introduce a new struct REveJsonWrapper that just wraps a json object (after including the full nlohmann/json.hpp). As the struct is under our control, we can easily forward declare the type and use it for method arguments.

Since nlohmann/json#3590, the basic_json class and the json using-declaration are located in a "versioned, ABI-tagged inline namespace". This makes it impossible to forward declare the type in REveElement.hxx. Instead introduce a new struct REveJsonWrapper that just wraps a json object (after including the full nlohmann/json.hpp). As the struct is under our control, we can easily forward declare the type and use it for method arguments. Fixes root-project#11130

falbrechtskirchinger changed the title ~~Add versioned inline namespace~~ Add versioned, ABI-tagged inline namespace and add namespace macros Jul 19, 2022

falbrechtskirchinger force-pushed the inline-ns branch from a3afc53 to 47dbf10 Compare July 19, 2022 07:34

falbrechtskirchinger mentioned this pull request Jul 19, 2022

Improve unit testing (Part 2) #3385

Draft

2 tasks

falbrechtskirchinger changed the title ~~Add versioned, ABI-tagged inline namespace and add namespace macros~~ Add versioned, ABI-tagged inline namespace and namespace macros Jul 19, 2022

falbrechtskirchinger force-pushed the inline-ns branch 3 times, most recently from c27d0d2 to 57f7dc0 Compare July 19, 2022 13:36

falbrechtskirchinger commented Jul 19, 2022

View reviewed changes

falbrechtskirchinger marked this pull request as ready for review July 19, 2022 14:03

falbrechtskirchinger requested a review from nlohmann as a code owner July 19, 2022 14:03

gregmarr reviewed Jul 19, 2022

View reviewed changes

This was linked to issues Jul 20, 2022

Using versioned namespaces #1539

Closed

Consider putting the user-defined literals in a namespace #1682

Closed

nlohmann requested changes Jul 20, 2022

View reviewed changes

tests/abi/CMakeLists.txt Show resolved Hide resolved

nlohmann added this to the Release 3.11.0 milestone Jul 23, 2022

falbrechtskirchinger force-pushed the inline-ns branch 3 times, most recently from dd0c855 to 01f6c23 Compare July 24, 2022 09:19

nlohmann removed a link to an issue Jul 28, 2022

Consider putting the user-defined literals in a namespace #1682

Closed

falbrechtskirchinger added 10 commits July 29, 2022 14:25

Add versioned inline namespace

bfa5f9a

Add a versioned inline namespace to prevent ABI issues when linking code using multiple library versions.

Add namespace macros

04ff891

Encode ABI information in inline namespace

adbd67f

Add _diag suffix to inline namespace if JSON_DIAGNOSTICS is enabled, and _ldvcmp suffix if JSON_USE_LEGACY_DISCARDED_VALUE_COMPARISON is enabled.

Move ABI-affecting macros into abi_macros.hpp

4fe45ab

Move std_fs namespace definition into std_fs.hpp

db63cde

Remove std_fs namespace from unit test

5bbcb6c

Format more files in tests directory

f42b31b

Add unit tests

9a66df2

Update documentation

6adc7a8

Fix GDB pretty printer

478fded

falbrechtskirchinger force-pushed the inline-ns branch from a88aa79 to 478fded Compare July 29, 2022 12:30

fixup! Add namespace macros

bd39ad6

falbrechtskirchinger mentioned this pull request Jul 29, 2022

More documentation updates for 3.11.0 #3553

Merged

6 tasks

nlohmann reviewed Jul 29, 2022

View reviewed changes

include/nlohmann/detail/abi_macros.hpp Outdated Show resolved Hide resolved

Derive ABI prefix from NLOHMANN_JSON_VERSION_*

0ac4c8a

nlohmann reviewed Jul 30, 2022

View reviewed changes

include/nlohmann/detail/abi_macros.hpp Show resolved Hide resolved

nlohmann approved these changes Jul 30, 2022

View reviewed changes

nlohmann self-assigned this Jul 30, 2022

nlohmann added the release item: ⚡ improvement label Jul 30, 2022

nlohmann merged commit d909f80 into nlohmann:develop Jul 30, 2022

falbrechtskirchinger deleted the inline-ns branch July 30, 2022 20:02

michaeldurland mentioned this pull request Aug 2, 2022

json_fwd.hpp no longer standalone #3656

Closed

2 tasks

guitargeek mentioned this pull request Aug 8, 2022

ROOT doesn't compile with new nlohmann-json version 3.11.0 root-project/root#11130

Closed

jheydebrand mentioned this pull request Aug 11, 2022

MSVC natvis visualizer does not work after introduction of inline ABI namespace #3696

Closed

2 tasks

hahnjo mentioned this pull request Aug 16, 2022

Replace fwd decl of json with wrapper type root-project/root#11196

Closed

garethsb mentioned this pull request Nov 23, 2022

json-schema-validator: make sure to mark transitive requirement headers conan-io/conan-center-index#14353

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add versioned, ABI-tagged inline namespace and namespace macros #3590

Add versioned, ABI-tagged inline namespace and namespace macros #3590

falbrechtskirchinger commented Jul 18, 2022 •

edited

Loading

coveralls commented Jul 18, 2022 •

edited

Loading

gregmarr commented Jul 18, 2022

falbrechtskirchinger commented Jul 19, 2022

falbrechtskirchinger commented Jul 19, 2022 •

edited

Loading

falbrechtskirchinger commented Jul 19, 2022

gregmarr left a comment

gregmarr commented Jul 19, 2022

falbrechtskirchinger commented Jul 19, 2022

falbrechtskirchinger commented Jul 19, 2022

gregmarr commented Jul 19, 2022

falbrechtskirchinger commented Jul 19, 2022 •

edited

Loading

nlohmann commented Jul 20, 2022

falbrechtskirchinger commented Jul 20, 2022

falbrechtskirchinger commented Jul 29, 2022

nlohmann left a comment

Add versioned, ABI-tagged inline namespace and namespace macros #3590

Add versioned, ABI-tagged inline namespace and namespace macros #3590

Conversation

falbrechtskirchinger commented Jul 18, 2022 • edited Loading

To Do:

coveralls commented Jul 18, 2022 • edited Loading

gregmarr commented Jul 18, 2022

falbrechtskirchinger commented Jul 19, 2022

falbrechtskirchinger commented Jul 19, 2022 • edited Loading

falbrechtskirchinger commented Jul 19, 2022

gregmarr left a comment

Choose a reason for hiding this comment

gregmarr commented Jul 19, 2022

falbrechtskirchinger commented Jul 19, 2022

falbrechtskirchinger commented Jul 19, 2022

gregmarr commented Jul 19, 2022

falbrechtskirchinger commented Jul 19, 2022 • edited Loading

nlohmann commented Jul 20, 2022

falbrechtskirchinger commented Jul 20, 2022

falbrechtskirchinger commented Jul 29, 2022

nlohmann left a comment

Choose a reason for hiding this comment

falbrechtskirchinger commented Jul 18, 2022 •

edited

Loading

coveralls commented Jul 18, 2022 •

edited

Loading

falbrechtskirchinger commented Jul 19, 2022 •

edited

Loading

falbrechtskirchinger commented Jul 19, 2022 •

edited

Loading