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

Replace fwd decl of json with wrapper type #11196

Closed
wants to merge 1 commit into from

Conversation

hahnjo
Copy link
Member

@hahnjo hahnjo commented Aug 16, 2022

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 #11130

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
@hahnjo hahnjo requested a review from Axel-Naumann August 16, 2022 13:51
@hahnjo hahnjo self-assigned this Aug 16, 2022
@hahnjo hahnjo requested review from osschar and linev as code owners August 16, 2022 13:51
@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

@hahnjo hahnjo marked this pull request as draft August 16, 2022 13:51
@hahnjo
Copy link
Member Author

hahnjo commented Aug 16, 2022

Two issues with this approach:

  1. It's not fully API compatible with the old argument types (cannot bind to references of a different type)
  2. Users cannot overwrite the functions anymore; if that's needed, we can make REveJsonWrapper.hxx public while keeping it out of the module (?).

@linev
Copy link
Member

linev commented Aug 16, 2022

Problem is not only eve7 - where derived classes used in examples.

nlohmann/json used in TBufferJSON and planned for usage with TJSONFile.
There much more operations used with it.

Idea to wait for llvm13 where one fully can avoid forward declarations and use directly nlohmann::json

@hahnjo
Copy link
Member Author

hahnjo commented Aug 16, 2022

@linev no, usage of nlohmann/json in source / implementation files is completely fine. The only problem is with its usage in headers -> modules. Regarding llvm13, I still don't understand; see my comment #11130 (comment)

@hahnjo
Copy link
Member Author

hahnjo commented Aug 19, 2022

Not the right approach as we might want to have APIs that take nlohmann::json arguments; #11130 provides a fix for the current problems with versions 3.11+

@hahnjo hahnjo closed this Aug 19, 2022
@hahnjo hahnjo deleted the revejsonwrapper branch August 19, 2022 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ROOT doesn't compile with new nlohmann-json version 3.11.0
3 participants