Skip to content

Commit c1dab64

Browse files
ktfMichaelLettrich
ktf
authored and
MichaelLettrich
committed
DPL: streamline DataAllocator::make (#1970)
This uses C++ if constexpr to simplify and unify the `DataAllocator::make` logic, hopefully resulting in more readable code. Notice how it uses `decltype(auto)` to deduce the return type from the expression. The original commit 6b82ff3 has an exhaustive description of why this is needed.
1 parent 5070f41 commit c1dab64

3 files changed

Lines changed: 99 additions & 152 deletions

File tree

‎Framework/Core/include/Framework/DataAllocator.h‎

Lines changed: 72 additions & 152 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,21 @@ namespace framework
6161
{
6262
class ContextRegistry;
6363

64+
namespace
65+
{
66+
template <typename T>
67+
struct type_dependent : std::false_type {
68+
};
69+
} // namespace
70+
71+
#define ERROR_STRING \
72+
"data type T not supported by API, " \
73+
"\n specializations available for" \
74+
"\n - trivially copyable, non-polymorphic structures" \
75+
"\n - arrays of those" \
76+
"\n - TObject with additional constructor arguments" \
77+
"\n - Classes and structs with boost serialization support" \
78+
"\n - std containers of those"
6479
/// This allocator is responsible to make sure that the messages created match
6580
/// the provided spec and that depending on how many pipelined reader we
6681
/// have, messages get created on the channel for the reader of the current
@@ -84,98 +99,64 @@ class DataAllocator
8499

85100
void adoptChunk(const Output&, char*, size_t, fairmq_free_fn*, void*);
86101

87-
// In case no extra argument is provided and the passed type is trivially
88-
// copyable and non polymorphic, the most likely wanted behavior is to create
89-
// a message with that type, and so we do.
90-
template <typename T>
91-
typename std::enable_if<is_messageable<T>::value == true, T&>::type
92-
make(const Output& spec)
93-
{
94-
return *reinterpret_cast<T*>(newChunk(spec, sizeof(T)).data());
95-
}
96-
97-
// In case an extra argument is provided, we consider this an array /
98-
// collection elements of that type
99-
// FIXME: once the vector functionality with polymorphic allocator is fully in place, this might be dropped
100-
template <typename T>
101-
typename std::enable_if<is_messageable<T>::value == true, gsl::span<T>&>::type
102-
make(const Output& spec, size_t nElements)
103-
{
104-
auto size = nElements * sizeof(T);
105-
std::string channel = matchDataHeader(spec, mTimingInfo->timeslice);
106-
auto context = mContextRegistry->get<MessageContext>();
107-
108-
FairMQMessagePtr headerMessage = headerMessageFromOutput(spec, channel, o2::header::gSerializationMethodNone, size);
109-
return context->add<MessageContext::SpanObject<T>>(std::move(headerMessage), channel, 0, nElements).get();
110-
}
111-
112-
/// Use this in case you want to leave the creation
113-
/// of a TObject to be transmitted to the framework.
114-
/// @a spec is the specification for the output
115-
/// @a args is the arguments for the constructor of T
116-
/// @return a reference to the constructed object. Such an object
117-
/// will be sent to all the consumers of the output @a spec
118-
/// once the processing callback completes.
119-
template <typename T, typename... Args>
120-
typename std::enable_if<std::is_base_of<TObject, T>::value == true, T&>::type
121-
make(const Output& spec, Args... args)
122-
{
123-
auto obj = new T(args...);
124-
adopt(spec, obj);
125-
return *obj;
126-
}
127-
128-
/// Helper to create an std::string which will be owned by the framework
129-
/// and transmitted when the processing finishes.
130-
template <typename T, typename... Args>
131-
typename std::enable_if<std::is_base_of<std::string, T>::value == true, T&>::type
132-
make(const Output& spec, Args... args)
133-
{
134-
std::string* s = new std::string(args...);
135-
adopt(spec, s);
136-
return *s;
137-
}
138-
139-
/// Helper to create a TableBuilder which will be owned by the framework
140-
/// FIXME: perfect forwarding?
102+
/// Generic helper to create an object which is owned by the framework and
103+
/// returned as a reference to the own object.
104+
/// Note: decltype(auto) will deduce the return type from the expression and it
105+
/// will be lvalue reference for the framework-owned objects. Instances of local
106+
/// variables like shared_ptr will be returned by value/move/return value optimization.
107+
/// Objects created this way will be sent to the channel specified by @spec
141108
template <typename T, typename... Args>
142-
typename std::enable_if<std::is_base_of<TableBuilder, T>::value == true, T&>::type
143-
make(const Output& spec, Args... args)
109+
decltype(auto) make(const Output& spec, Args... args)
144110
{
145-
TableBuilder* tb = new TableBuilder(args...);
146-
adopt(spec, tb);
147-
return *tb;
148-
}
149-
150-
/// Helper to create a arrow::ipc::RecordBatchWriter, owned by the framework
151-
/// which creates record batches with the given @a schema in a FairMQMessage.
152-
template <typename T>
153-
typename std::enable_if_t<std::is_base_of_v<arrow::ipc::RecordBatchWriter, T> == true, std::shared_ptr<T>>
154-
make(const Output& spec, std::shared_ptr<arrow::Schema> schema)
155-
{
156-
std::shared_ptr<arrow::ipc::RecordBatchWriter> writer;
157-
create(spec, &writer, schema);
158-
return writer;
159-
}
160-
161-
/// Helper to create an byte stream buffer using boost serialization, which will be owned by the framework
162-
/// and transmitted when the processing finishes.
163-
template <typename T, typename WT = typename T::wrapped_type>
164-
typename std::enable_if<is_specialization<T, BoostSerialized>::value == true, WT&>::type
165-
make(const Output& specs)
166-
{
167-
return make_boost<WT>(std::move(specs));
168-
}
169-
170-
template <typename T>
171-
typename std::enable_if<is_specialization<T, BoostSerialized>::value == false //
172-
&& is_messageable<T>::value == false //
173-
&& framework::is_boost_serializable<T>::value == true //
174-
&& std::is_base_of<std::string, T>::value == false,
175-
T&>::type
176-
make(const Output& spec)
177-
{
178-
return make_boost<T>(std::move(spec));
111+
if constexpr (std::is_base_of_v<TObject, T>) {
112+
auto obj = new T(args...);
113+
adopt(spec, obj);
114+
return *obj;
115+
} else if constexpr (std::is_base_of_v<std::string, T>) {
116+
std::string* s = new std::string(args...);
117+
adopt(spec, s);
118+
return *s;
119+
} else if constexpr (std::is_base_of_v<TableBuilder, T>) {
120+
TableBuilder* tb = new TableBuilder(args...);
121+
adopt(spec, tb);
122+
return *tb;
123+
} else if constexpr (sizeof...(Args) == 0) {
124+
if constexpr (is_messageable<T>::value == true) {
125+
return *reinterpret_cast<T*>(newChunk(spec, sizeof(T)).data());
126+
} else if constexpr (is_specialization<T, BoostSerialized>::value == true) {
127+
return make_boost<typename T::wrapped_type>(std::move(spec));
128+
} else if constexpr (is_specialization<T, BoostSerialized>::value == false && framework::is_boost_serializable<T>::value == true && std::is_base_of<std::string, T>::value == false) {
129+
return make_boost<T>(std::move(spec));
130+
} else {
131+
static_assert(type_dependent<T>::value, ERROR_STRING);
132+
}
133+
} else if constexpr (sizeof...(Args) == 1) {
134+
using FirstArg = typename std::tuple_element<0, std::tuple<Args...>>::type;
135+
if constexpr (std::is_integral_v<FirstArg>) {
136+
if constexpr (is_messageable<T>::value == true) {
137+
auto [nElements] = std::make_tuple(args...);
138+
auto size = nElements * sizeof(T);
139+
std::string channel = matchDataHeader(spec, mTimingInfo->timeslice);
140+
auto context = mContextRegistry->get<MessageContext>();
141+
142+
FairMQMessagePtr headerMessage = headerMessageFromOutput(spec, channel, o2::header::gSerializationMethodNone, size);
143+
return context->add<MessageContext::SpanObject<T>>(std::move(headerMessage), channel, 0, nElements).get();
144+
}
145+
} else if constexpr (std::is_same_v<FirstArg, std::shared_ptr<arrow::Schema>>) {
146+
if constexpr (std::is_base_of_v<arrow::ipc::RecordBatchWriter, T>) {
147+
auto [schema] = std::make_tuple(args...);
148+
std::shared_ptr<arrow::ipc::RecordBatchWriter> writer;
149+
create(spec, &writer, schema);
150+
return writer;
151+
}
152+
} else if constexpr (is_specialization<T, BoostSerialized>::value) {
153+
return make_boost<FirstArg>(std::move(spec));
154+
} else {
155+
static_assert(type_dependent<T>::value, ERROR_STRING);
156+
}
157+
} else {
158+
static_assert(type_dependent<T>::value, ERROR_STRING);
159+
}
179160
}
180161

181162
template <typename T>
@@ -186,67 +167,6 @@ class DataAllocator
186167
return *buff;
187168
}
188169

189-
/// catching unsupported type for case without additional arguments
190-
/// have to add three specializations because of the different role of
191-
/// the arguments and the different return types
192-
template <typename T>
193-
typename std::enable_if<
194-
is_specialization<T, BoostSerialized>::value == false //
195-
&& std::is_base_of<TObject, T>::value == false //
196-
&& std::is_base_of<TableBuilder, T>::value == false //
197-
&& is_messageable<T>::value == false //
198-
&& std::is_same<std::string, T>::value == false //
199-
&& framework::is_boost_serializable<T>::value == false, //
200-
T&>::type
201-
make(const Output&)
202-
{
203-
static_assert(always_static_assert_v<T>,
204-
"data type T not supported by API, \n specializations available for"
205-
"\n - trivially copyable, non-polymorphic structures"
206-
"\n - arrays of those"
207-
"\n - TObject with additional constructor arguments"
208-
"\n - Classes and structs with boost serialization support"
209-
"\n - std containers of those");
210-
}
211-
212-
/// catching unsupported type for case of span of objects
213-
template <typename T>
214-
typename std::enable_if<
215-
std::is_base_of<TObject, T>::value == false //
216-
&& is_messageable<T>::value == false //
217-
&& std::is_same<std::string, T>::value == false //
218-
&& framework::is_boost_serializable<T>::value == false, //
219-
gsl::span<T>>::type
220-
make(const Output&, size_t)
221-
{
222-
static_assert(always_static_assert_v<T>,
223-
"data type T not supported by API, \n specializations available for"
224-
"\n - trivially copyable, non-polymorphic structures"
225-
"\n - arrays of those"
226-
"\n - TObject with additional constructor arguments"
227-
"\n - Classes and structs with boost serialization support"
228-
"\n - std containers of those");
229-
}
230-
231-
/// catching unsupported type for case of at least two additional arguments
232-
template <typename T, typename U, typename V, typename... Args>
233-
typename std::enable_if<
234-
std::is_base_of<TObject, T>::value == false //
235-
&& is_messageable<T>::value == false //
236-
&& std::is_same<std::string, T>::value == false //
237-
&& framework::is_boost_serializable<T>::value == false, //
238-
T&>::type
239-
make(const Output&, U, V, Args...)
240-
{
241-
static_assert(always_static_assert_v<T>,
242-
"data type T not supported by API, \n specializations available for"
243-
"\n - trivially copyable, non-polymorphic structures"
244-
"\n - arrays of those"
245-
"\n - TObject with additional constructor arguments"
246-
"\n - Classes and structs with boost serialization support"
247-
"\n - std containers of those");
248-
}
249-
250170
/// Adopt a TObject in the framework and serialize / send
251171
/// it to the consumers of @a spec once done.
252172
void
@@ -476,7 +396,7 @@ class DataAllocator
476396
/// OutputRef descriptors are expected to be passed as rvalue, i.e. a temporary object in the
477397
/// function call
478398
template <typename T, typename... Args>
479-
auto& make(OutputRef&& ref, Args&&... args)
399+
decltype(auto) make(OutputRef&& ref, Args&&... args)
480400
{
481401
return make<T>(getOutputByBind(std::move(ref)), std::forward<Args>(args)...);
482402
}

‎Framework/Core/test/test_DataAllocator.cxx‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
#include "Framework/ControlService.h"
1919
#include "Framework/RawDeviceService.h"
2020
#include "Framework/SerializationMethods.h"
21+
#include "Framework/OutputRoute.h"
2122
#include "Headers/DataHeader.h"
2223
#include "TestClasses.h"
2324
#include "Framework/Logger.h"
@@ -32,6 +33,19 @@ using namespace o2::framework;
3233
LOG(ERROR) << R"(Test condition ")" #condition R"(" failed)"; \
3334
}
3435

36+
// this function is only used to do the static checks for API return types
37+
void doTypeChecks()
38+
{
39+
TimingInfo* timingInfo = nullptr;
40+
ContextRegistry* contextes = nullptr;
41+
std::vector<OutputRoute> routes;
42+
DataAllocator allocator(timingInfo, contextes, routes);
43+
const Output output{ "TST", "DUMMY", 0, Lifetime::Timeframe };
44+
// we require references to objects owned by allocator context
45+
static_assert(std::is_lvalue_reference<decltype(allocator.make<int>(output))>::value);
46+
static_assert(std::is_lvalue_reference<decltype(allocator.make<std::string>(output, "test"))>::value);
47+
}
48+
3549
namespace test
3650
{
3751
struct MetaHeader : public o2::header::BaseHeader {

‎Framework/Core/test/test_TableBuilder.cxx‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616

1717
#include "Framework/TableBuilder.h"
1818
#include "Framework/TableConsumer.h"
19+
#include "Framework/DataAllocator.h"
20+
#include "Framework/OutputRoute.h"
1921
#include <arrow/table.h>
2022
#include <ROOT/RDataFrame.hxx>
2123
#include <ROOT/RArrowDS.hxx>
@@ -247,3 +249,14 @@ BOOST_AUTO_TEST_CASE(TestSoAIntegration)
247249
++i;
248250
}
249251
}
252+
253+
BOOST_AUTO_TEST_CASE(TestDataAllocatorReturnType)
254+
{
255+
TimingInfo* timingInfo = nullptr;
256+
ContextRegistry* contextes = nullptr;
257+
std::vector<OutputRoute> routes;
258+
DataAllocator allocator(timingInfo, contextes, routes);
259+
const Output output{ "TST", "DUMMY", 0, Lifetime::Timeframe };
260+
// we require reference to object owned by allocator context
261+
static_assert(std::is_lvalue_reference<decltype(allocator.make<TableBuilder>(output))>::value);
262+
}

0 commit comments

Comments
 (0)