From fd61e16f1d2bf07adcf7c1628f2df610b3db97f7 Mon Sep 17 00:00:00 2001 From: Giulio Eulisse <10544+ktf@users.noreply.github.com> Date: Tue, 6 May 2025 11:07:29 +0200 Subject: [PATCH 1/2] DPL: improve DataSpecUtils::describe API in case of buffers Just like snprintf, it makes sense to return the size of the formatted output. --- .../Core/include/Framework/DataSpecUtils.h | 17 +++--- Framework/Core/src/DataSpecUtils.cxx | 52 ++++++++---------- .../Core/test/unittest_DataSpecUtils.cxx | 55 +++++++++++++++++++ 3 files changed, 85 insertions(+), 39 deletions(-) diff --git a/Framework/Core/include/Framework/DataSpecUtils.h b/Framework/Core/include/Framework/DataSpecUtils.h index 65f8585302aa7..a98fb894d7cbd 100644 --- a/Framework/Core/include/Framework/DataSpecUtils.h +++ b/Framework/Core/include/Framework/DataSpecUtils.h @@ -18,11 +18,12 @@ #include -namespace o2 -{ -namespace framework +namespace o2::framework { +template +concept HasMatcher = requires (T& t) { t.matcher; }; + struct DataSpecUtils { /// @return true if a given InputSpec @a spec matches with a @a target ConcreteDataMatcher static bool match(InputSpec const& spec, ConcreteDataMatcher const& target); @@ -152,10 +153,8 @@ struct DataSpecUtils { static bool validate(OutputSpec const& output); /// Same as the other describe, but uses a buffer to reduce memory churn. - static void describe(char* buffer, size_t size, InputSpec const& spec); - - /// Same as the other describe, but uses a buffer to reduce memory churn. - static void describe(char* buffer, size_t size, OutputSpec const& spec); + template + static size_t describe(char* buffer, size_t size, T const& spec); /// If possible extract the ConcreteDataMatcher from an InputSpec. This /// can be done either if the InputSpec is defined in terms for a ConcreteDataMatcher @@ -250,6 +249,6 @@ struct DataSpecUtils { static void updateOutputList(std::vector& list, OutputSpec&& input); }; -} // namespace framework -} // namespace o2 +} // namespace o2::framework + #endif // FRAMEWORK_DATASPECUTILS_H diff --git a/Framework/Core/src/DataSpecUtils.cxx b/Framework/Core/src/DataSpecUtils.cxx index 3babbaba2a6ca..88cbcc259cb73 100644 --- a/Framework/Core/src/DataSpecUtils.cxx +++ b/Framework/Core/src/DataSpecUtils.cxx @@ -15,11 +15,13 @@ #include "Framework/RuntimeError.h" #include "Headers/DataHeaderHelpers.h" +#include #include #include #include #include #include +#include namespace o2::framework { @@ -87,39 +89,29 @@ std::string DataSpecUtils::describe(OutputSpec const& spec) spec.matcher); } -void DataSpecUtils::describe(char* buffer, size_t size, InputSpec const& spec) +template +size_t DataSpecUtils::describe(char* buffer, size_t size, T const& spec) { - if (auto concrete = std::get_if(&spec.matcher)) { - char origin[5]; - origin[4] = 0; - char description[17]; - description[16] = 0; - snprintf(buffer, size, "%s/%s/%" PRIu32, (strncpy(origin, concrete->origin.str, 4), origin), - (strncpy(description, concrete->description.str, 16), description), concrete->subSpec); - } else if (auto matcher = std::get_if(&spec.matcher)) { - std::ostringstream ss; - ss << ""; - strncpy(buffer, ss.str().c_str(), size - 1); - } else { - throw runtime_error("Unsupported InputSpec"); - } + auto result = std::visit(overloaded{ + [buffer, size](ConcreteDataMatcher const& concrete) -> fmt::format_to_n_result { + return fmt::format_to_n(buffer, size - 1, "{:.4}/{:.16}/{}", concrete.origin.str, concrete.description.str, concrete.subSpec); + }, + [buffer, size](ConcreteDataTypeMatcher const& concrete) -> fmt::format_to_n_result { + return fmt::format_to_n(buffer, size - 1, "", concrete.origin, concrete.description); + }, + [buffer, size](DataDescriptorMatcher const& matcher) -> fmt::format_to_n_result { + std::ostringstream ss; + ss << ""; + return fmt::format_to_n(buffer, size - 1, "{}", ss.str()); + }, + [](...) -> fmt::format_to_n_result { throw std::runtime_error("Unsupported Input / Output Spec"); } + }, spec.matcher); + *result.out = '\0'; + return result.out - buffer; } -void DataSpecUtils::describe(char* buffer, size_t size, OutputSpec const& spec) -{ - if (auto concrete = std::get_if(&spec.matcher)) { - char origin[5]; - origin[4] = 0; - char description[17]; - description[16] = 0; - snprintf(buffer, size, "%s/%s/%" PRIu32, (strncpy(origin, concrete->origin.str, 4), origin), - (strncpy(description, concrete->description.str, 16), description), concrete->subSpec); - } else if (auto concrete = std::get_if(&spec.matcher)) { - fmt::format_to(buffer, "", concrete->origin, concrete->description); - } else { - throw runtime_error("Unsupported OutputSpec"); - } -} +template size_t DataSpecUtils::describe(char* buffer, size_t size, InputSpec const& spec); +template size_t DataSpecUtils::describe(char* buffer, size_t size, OutputSpec const& spec); std::string DataSpecUtils::label(InputSpec const& spec) { diff --git a/Framework/Core/test/unittest_DataSpecUtils.cxx b/Framework/Core/test/unittest_DataSpecUtils.cxx index e6b2f4a22c018..833251cf16bb9 100644 --- a/Framework/Core/test/unittest_DataSpecUtils.cxx +++ b/Framework/Core/test/unittest_DataSpecUtils.cxx @@ -42,6 +42,7 @@ TEST_CASE("ConcreteData") CHECK(std::string(concrete.description.as()) == "FOOO"); CHECK(concrete.subSpec == 1); CHECK(DataSpecUtils::describe(spec) == "TEST/FOOO/1"); + CHECK(DataSpecUtils::describe(spec) == "TEST/FOOO/1"); CHECK(*DataSpecUtils::getOptionalSubSpec(spec) == 1); ConcreteDataTypeMatcher dataType = DataSpecUtils::asConcreteDataTypeMatcher(spec); @@ -59,6 +60,44 @@ TEST_CASE("ConcreteData") } } +TEST_CASE("DescribeUsingBuffer") +{ + o2::framework::clean_all_runtime_errors(); + OutputSpec spec{ + "TEST", + "FOOO", + 1, + Lifetime::Timeframe}; + + InputSpec inputSpec{ + "binding", + "TEST", + "FOOO", + 1, + Lifetime::Timeframe}; + + REQUIRE(DataSpecUtils::validate(inputSpec)); + + { + char buffer[1024]; + + ConcreteDataMatcher concrete = DataSpecUtils::asConcreteDataMatcher(spec); + CHECK(std::string(concrete.origin.as()) == "TEST"); + CHECK(std::string(concrete.description.as()) == "FOOO"); + CHECK(concrete.subSpec == 1); + auto size = DataSpecUtils::describe(buffer, 1024, spec); + CHECK(std::string_view(buffer, size) == "TEST/FOOO/1"); + size = DataSpecUtils::describe(buffer, 1024, spec); + CHECK(std::string_view(buffer, size) == "TEST/FOOO/1"); + CHECK(*DataSpecUtils::getOptionalSubSpec(spec) == 1); + + char buffer2[1024]; + size = DataSpecUtils::describe(buffer2, 5, spec); + // We always nullterminate the string + CHECK(std::string_view(buffer2, size) == "TEST"); + } +} + TEST_CASE("WithWildCards") { OutputSpec spec{ @@ -78,6 +117,22 @@ TEST_CASE("WithWildCards") CHECK(DataSpecUtils::getOptionalSubSpec(spec) == std::nullopt); } +TEST_CASE("WithWildCardsBuffer") +{ + char buffer[1024]; + OutputSpec spec{ + {"TEST", "FOOO"}, + Lifetime::Timeframe}; + + auto size = DataSpecUtils::describe(buffer, 1024, spec); + CHECK(std::string_view(buffer, size) == ""); + + char buffer2[1024]; + size = DataSpecUtils::describe(buffer2, 5, spec); + // We always null terminate the buffer. + CHECK( std::string_view(buffer2, size) == " Date: Tue, 6 May 2025 10:10:11 +0000 Subject: [PATCH 2/2] Please consider the following formatting changes --- .../Core/include/Framework/DataSpecUtils.h | 2 +- Framework/Core/src/DataSpecUtils.cxx | 26 +++++++++---------- .../Core/test/unittest_DataSpecUtils.cxx | 2 +- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/Framework/Core/include/Framework/DataSpecUtils.h b/Framework/Core/include/Framework/DataSpecUtils.h index a98fb894d7cbd..588aa30da7e08 100644 --- a/Framework/Core/include/Framework/DataSpecUtils.h +++ b/Framework/Core/include/Framework/DataSpecUtils.h @@ -22,7 +22,7 @@ namespace o2::framework { template -concept HasMatcher = requires (T& t) { t.matcher; }; +concept HasMatcher = requires(T& t) { t.matcher; }; struct DataSpecUtils { /// @return true if a given InputSpec @a spec matches with a @a target ConcreteDataMatcher diff --git a/Framework/Core/src/DataSpecUtils.cxx b/Framework/Core/src/DataSpecUtils.cxx index 88cbcc259cb73..48f5e6abcad5b 100644 --- a/Framework/Core/src/DataSpecUtils.cxx +++ b/Framework/Core/src/DataSpecUtils.cxx @@ -93,19 +93,19 @@ template size_t DataSpecUtils::describe(char* buffer, size_t size, T const& spec) { auto result = std::visit(overloaded{ - [buffer, size](ConcreteDataMatcher const& concrete) -> fmt::format_to_n_result { - return fmt::format_to_n(buffer, size - 1, "{:.4}/{:.16}/{}", concrete.origin.str, concrete.description.str, concrete.subSpec); - }, - [buffer, size](ConcreteDataTypeMatcher const& concrete) -> fmt::format_to_n_result { - return fmt::format_to_n(buffer, size - 1, "", concrete.origin, concrete.description); - }, - [buffer, size](DataDescriptorMatcher const& matcher) -> fmt::format_to_n_result { - std::ostringstream ss; - ss << ""; - return fmt::format_to_n(buffer, size - 1, "{}", ss.str()); - }, - [](...) -> fmt::format_to_n_result { throw std::runtime_error("Unsupported Input / Output Spec"); } - }, spec.matcher); + [buffer, size](ConcreteDataMatcher const& concrete) -> fmt::format_to_n_result { + return fmt::format_to_n(buffer, size - 1, "{:.4}/{:.16}/{}", concrete.origin.str, concrete.description.str, concrete.subSpec); + }, + [buffer, size](ConcreteDataTypeMatcher const& concrete) -> fmt::format_to_n_result { + return fmt::format_to_n(buffer, size - 1, "", concrete.origin, concrete.description); + }, + [buffer, size](DataDescriptorMatcher const& matcher) -> fmt::format_to_n_result { + std::ostringstream ss; + ss << ""; + return fmt::format_to_n(buffer, size - 1, "{}", ss.str()); + }, + [](...) -> fmt::format_to_n_result { throw std::runtime_error("Unsupported Input / Output Spec"); }}, + spec.matcher); *result.out = '\0'; return result.out - buffer; } diff --git a/Framework/Core/test/unittest_DataSpecUtils.cxx b/Framework/Core/test/unittest_DataSpecUtils.cxx index 833251cf16bb9..6128183aefa11 100644 --- a/Framework/Core/test/unittest_DataSpecUtils.cxx +++ b/Framework/Core/test/unittest_DataSpecUtils.cxx @@ -130,7 +130,7 @@ TEST_CASE("WithWildCardsBuffer") char buffer2[1024]; size = DataSpecUtils::describe(buffer2, 5, spec); // We always null terminate the buffer. - CHECK( std::string_view(buffer2, size) == "