From f20eafc5e341bce18743017be14d95886f9eb18c Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Sat, 11 Nov 2023 13:14:02 +0100 Subject: [PATCH] [DF] Avoid public dependency of RDataFrame on `nlohmann::json` It's better if the RDataFrame doesn't depend on `nlohmann::json` for this reason: If you build ROOT **without** the builtin nlohmannjson, it finds the json header with `find_package(nlohmann_json)`, which is fine. However, if the `nlohmann/json.hpp` header is not in the default include path, one is now forced to also do `find_package(nlohmann_json)` in every compiled project that uses RDataFrame! This happened to one of our students recently, which is why I got aware of the problem. On most Linux systems, this is not a problem because `json.hpp` is in a standard location, but for macOS this doesn't seem to be the case necessarily. --- tree/dataframe/CMakeLists.txt | 2 +- tree/dataframe/inc/ROOT/RDF/RMetaData.hxx | 28 ++++++++-- tree/dataframe/src/RMetaData.cxx | 65 +++++++++++++++-------- 3 files changed, 69 insertions(+), 26 deletions(-) diff --git a/tree/dataframe/CMakeLists.txt b/tree/dataframe/CMakeLists.txt index 2400734d8a973..19a80075fae16 100644 --- a/tree/dataframe/CMakeLists.txt +++ b/tree/dataframe/CMakeLists.txt @@ -164,7 +164,7 @@ endif() if(builtin_nlohmannjson) target_include_directories(ROOTDataFrame PRIVATE ${CMAKE_SOURCE_DIR}/builtins) else() - target_link_libraries(ROOTDataFrame PUBLIC nlohmann_json::nlohmann_json) + target_link_libraries(ROOTDataFrame PRIVATE nlohmann_json::nlohmann_json) endif() ROOT_ADD_TEST_SUBDIRECTORY(test) diff --git a/tree/dataframe/inc/ROOT/RDF/RMetaData.hxx b/tree/dataframe/inc/ROOT/RDF/RMetaData.hxx index e2a343d69459a..8a6e0028bd25c 100644 --- a/tree/dataframe/inc/ROOT/RDF/RMetaData.hxx +++ b/tree/dataframe/inc/ROOT/RDF/RMetaData.hxx @@ -11,10 +11,21 @@ #ifndef ROOT_RDF_RMETADATA #define ROOT_RDF_RMETADATA -#include #include +#include namespace ROOT { + +namespace Internal { +namespace RDF { +// To avoid unnecessary dependence on nlohman json in the interface. Note that +// we should not forward declare nlohmann::json directly, since its declaration +// might change (it is currently a typedef). With this wrapper type, we are +// completely decoupled on nlohmann::json in the RMetaData interface. +struct RMetaDataJson; +} +} + namespace RDF { namespace Experimental { @@ -27,9 +38,17 @@ namespace Experimental { Once a dataframe is built with RMetaData object, it could be accessed via DefinePerSample. */ class RMetaData { - nlohmann::json fJson; - public: + + RMetaData(); + // Note: each RMetaData instance should own its own fJson object, just as if + // the underlying nlohmann::json object would be owned by value. + RMetaData(RMetaData const&); + RMetaData(RMetaData &&); + RMetaData & operator=(RMetaData const&); + RMetaData & operator=(RMetaData &&); + ~RMetaData(); + void Add(const std::string &key, int val); void Add(const std::string &key, double val); void Add(const std::string &key, const std::string &val); @@ -41,6 +60,9 @@ public: int GetI(const std::string &key, int defaultVal) const; double GetD(const std::string &key, double defaultVal) const; const std::string GetS(const std::string &key, const std::string &defaultVal) const; + +private: + std::unique_ptr fJson; }; } // namespace Experimental diff --git a/tree/dataframe/src/RMetaData.cxx b/tree/dataframe/src/RMetaData.cxx index c887ba84d9c5e..5dae9c464abb2 100644 --- a/tree/dataframe/src/RMetaData.cxx +++ b/tree/dataframe/src/RMetaData.cxx @@ -9,84 +9,105 @@ *************************************************************************/ #include "ROOT/RDF/RMetaData.hxx" +#include #include // std::logic_error +struct ROOT::Internal::RDF::RMetaDataJson { + nlohmann::json payload; +}; + namespace ROOT { namespace RDF { namespace Experimental { +RMetaData::RMetaData() : fJson{std::make_unique()} {} + +RMetaData::RMetaData(RMetaData const &other) : fJson{std::make_unique(*other.fJson)} {} + +RMetaData::RMetaData(RMetaData &&) = default; + +RMetaData &RMetaData::operator=(RMetaData const &other) +{ + fJson = std::make_unique(*other.fJson); + return *this; +} + +RMetaData &RMetaData::operator=(RMetaData &&) = default; + +RMetaData::~RMetaData() = default; + void RMetaData::Add(const std::string &key, int val) { - fJson[key] = val; + fJson->payload[key] = val; } void RMetaData::Add(const std::string &key, double val) { - fJson[key] = val; + fJson->payload[key] = val; } void RMetaData::Add(const std::string &key, const std::string &val) { - fJson[key] = val; + fJson->payload[key] = val; } std::string RMetaData::Dump(const std::string &key) const { - return fJson[key].dump(); + return fJson->payload[key].dump(); } int RMetaData::GetI(const std::string &key) const { - if (!fJson.contains(key)) + if (!fJson->payload.contains(key)) throw std::logic_error("No key with name " + key + " in the metadata object."); - if (!fJson[key].is_number_integer()) + if (!fJson->payload[key].is_number_integer()) throw std::logic_error("Key " + key + " is not of type int."); - return fJson[key].get(); + return fJson->payload[key].get(); } double RMetaData::GetD(const std::string &key) const { - if (!fJson.contains(key)) + if (!fJson->payload.contains(key)) throw std::logic_error("No key with name " + key + " in the metadata object."); - if (!fJson[key].is_number_float()) + if (!fJson->payload[key].is_number_float()) throw std::logic_error("Key " + key + " is not of type double."); - return fJson[key].get(); + return fJson->payload[key].get(); } std::string RMetaData::GetS(const std::string &key) const { - if (!fJson.contains(key)) + if (!fJson->payload.contains(key)) throw std::logic_error("No key with name " + key + " in the metadata object."); - if (!fJson[key].is_string()) + if (!fJson->payload[key].is_string()) throw std::logic_error("Key " + key + " is not of type string."); - return fJson[key].get(); + return fJson->payload[key].get(); } int RMetaData::GetI(const std::string &key, int defaultVal) const { - if (!fJson.contains(key)) + if (!fJson->payload.contains(key)) return defaultVal; - if (!fJson[key].is_number_integer()) + if (!fJson->payload[key].is_number_integer()) throw std::logic_error("Key " + key + " is not of type int."); - return fJson[key].get(); + return fJson->payload[key].get(); } double RMetaData::GetD(const std::string &key, double defaultVal) const { - if (!fJson.contains(key)) + if (!fJson->payload.contains(key)) return defaultVal; - if (!fJson[key].is_number_float()) + if (!fJson->payload[key].is_number_float()) throw std::logic_error("Key " + key + " is not of type double."); - return fJson[key].get(); + return fJson->payload[key].get(); } const std::string RMetaData::GetS(const std::string &key, const std::string &defaultVal) const { - if (!fJson.contains(key)) + if (!fJson->payload.contains(key)) return defaultVal; - if (!fJson[key].is_string()) + if (!fJson->payload[key].is_string()) throw std::logic_error("Key " + key + " is not of type string."); - return fJson[key].get(); + return fJson->payload[key].get(); } } // namespace Experimental