Skip to content

Commit abe259d

Browse files
ktfktf
committed
DPL: cleanup Variant copying for ArrayString
Previous implementation was actually working by accident.
1 parent 1d620f2 commit abe259d

File tree

4 files changed

+75
-59
lines changed

4 files changed

+75
-59
lines changed

‎Framework/Core/include/Framework/Variant.h‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -274,13 +274,24 @@ struct variant_helper<std::vector<std::string>> {
274274
// Allocates a new store and copies into it.
275275
static void set(void* store, std::vector<std::string> value)
276276
{
277-
new (reinterpret_cast<std::vector<std::string>*>(store)) std::vector<std::string>{};
278-
*(reinterpret_cast<std::vector<std::string>*>(store)) = value;
277+
auto ptr = reinterpret_cast<std::vector<std::string>*>(store);
278+
new (ptr) std::vector<std::string>{value};
279279
}
280280

281281
static std::vector<std::string> const& get(const void* store) { return *(reinterpret_cast<std::vector<std::string> const*>(store)); }
282282
};
283283

284+
template <>
285+
struct variant_helper<std::string*> {
286+
static void set(void* store, std::string* values, size_t size)
287+
{
288+
auto ptr = reinterpret_cast<std::vector<std::string>*>(store);
289+
new (ptr) std::vector<std::string>{values, values + size};
290+
}
291+
292+
static std::string const* get(const void* store) { return (*(reinterpret_cast<std::vector<std::string> const*>(store))).data(); }
293+
};
294+
284295
template <>
285296
struct variant_helper<const char*> {
286297
static const char* get(const void* store) { return *reinterpret_cast<const char* const*>(store); }
@@ -360,6 +371,16 @@ class Variant
360371
return variant_helper<T>::get(&mStore);
361372
}
362373

374+
template <typename T>
375+
[[nodiscard]] std::string const* get() const
376+
requires(std::same_as<std::string*, T>)
377+
{
378+
if (mType != VariantType::ArrayString) {
379+
throw runtime_error_f("Variant::get: Mismatch between types %d %d.", mType, VariantType::ArrayString);
380+
}
381+
return variant_helper<T>::get(&mStore);
382+
}
383+
363384
template <typename T>
364385
void set(T value)
365386
{

‎Framework/Core/include/Framework/VariantPropertyTreeHelpers.h‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ boost::property_tree::ptree basicVectorToBranch(std::vector<T>&& values)
3636
}
3737

3838
template <typename T>
39-
boost::property_tree::ptree vectorToBranch(T* values, size_t size)
39+
boost::property_tree::ptree vectorToBranch(T const* values, size_t size)
4040
{
4141
boost::property_tree::ptree branch;
4242
branch.put_child("values", basicVectorToBranch(values, size));
@@ -150,17 +150,17 @@ extern template boost::property_tree::ptree o2::framework::basicVectorToBranch(f
150150
extern template boost::property_tree::ptree o2::framework::basicVectorToBranch(int*, size_t);
151151
extern template boost::property_tree::ptree o2::framework::basicVectorToBranch(double*, size_t);
152152
extern template boost::property_tree::ptree o2::framework::basicVectorToBranch(bool*, size_t);
153-
extern template boost::property_tree::ptree o2::framework::basicVectorToBranch(std::basic_string<char>*, size_t);
153+
extern template boost::property_tree::ptree o2::framework::basicVectorToBranch(std::basic_string<char> const*, size_t);
154154

155155
extern template boost::property_tree::ptree o2::framework::vectorToBranch(std::vector<float>&& values);
156156
extern template boost::property_tree::ptree o2::framework::vectorToBranch(std::vector<int>&& values);
157157
extern template boost::property_tree::ptree o2::framework::vectorToBranch(std::vector<double>&& values);
158158
extern template boost::property_tree::ptree o2::framework::vectorToBranch(std::vector<std::string>&& values);
159-
extern template boost::property_tree::ptree o2::framework::vectorToBranch(float*, size_t);
160-
extern template boost::property_tree::ptree o2::framework::vectorToBranch(int*, size_t);
161-
extern template boost::property_tree::ptree o2::framework::vectorToBranch(double*, size_t);
162-
extern template boost::property_tree::ptree o2::framework::vectorToBranch(bool*, size_t);
163-
extern template boost::property_tree::ptree o2::framework::vectorToBranch(std::basic_string<char>*, size_t);
159+
extern template boost::property_tree::ptree o2::framework::vectorToBranch(float const*, size_t);
160+
extern template boost::property_tree::ptree o2::framework::vectorToBranch(int const*, size_t);
161+
extern template boost::property_tree::ptree o2::framework::vectorToBranch(double const*, size_t);
162+
extern template boost::property_tree::ptree o2::framework::vectorToBranch(bool const*, size_t);
163+
extern template boost::property_tree::ptree o2::framework::vectorToBranch(std::basic_string<char> const*, size_t);
164164

165165
extern template boost::property_tree::ptree o2::framework::labeledArrayToBranch(o2::framework::LabeledArray<float>&& array);
166166
extern template boost::property_tree::ptree o2::framework::labeledArrayToBranch(o2::framework::LabeledArray<int>&& array);

‎Framework/Core/src/Variant.cxx‎

Lines changed: 39 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -89,29 +89,29 @@ Variant::Variant(const Variant& other) : mType(other.mType)
8989
// In case this is an array we need to duplicate it to avoid
9090
// double deletion.
9191
switch (mType) {
92-
case variant_trait_v<const char*>:
92+
case VariantType::String:
9393
mSize = other.mSize;
9494
variant_helper<const char*>::set(&mStore, other.get<const char*>());
9595
return;
96-
case variant_trait_v<int*>:
96+
case VariantType::ArrayInt:
9797
mSize = other.mSize;
9898
variant_helper<int*>::set(&mStore, other.get<int*>(), mSize);
9999
return;
100-
case variant_trait_v<float*>:
100+
case VariantType::ArrayFloat:
101101
mSize = other.mSize;
102102
variant_helper<float*>::set(&mStore, other.get<float*>(), mSize);
103103
return;
104-
case variant_trait_v<double*>:
104+
case VariantType::ArrayDouble:
105105
mSize = other.mSize;
106106
variant_helper<double*>::set(&mStore, other.get<double*>(), mSize);
107107
return;
108-
case variant_trait_v<bool*>:
108+
case VariantType::ArrayBool:
109109
mSize = other.mSize;
110110
variant_helper<bool*>::set(&mStore, other.get<bool*>(), mSize);
111111
return;
112-
case variant_trait_v<std::string*>:
112+
case VariantType::ArrayString:
113113
mSize = other.mSize;
114-
variant_helper<std::string*>::set(&mStore, other.get<std::string*>(), mSize);
114+
variant_helper<std::vector<std::string>>::set(&mStore, other.get<std::vector<std::string>>());
115115
return;
116116
default:
117117
mStore = other.mStore;
@@ -124,23 +124,14 @@ Variant::Variant(Variant&& other) noexcept : mType(other.mType)
124124
mStore = other.mStore;
125125
mSize = other.mSize;
126126
switch (mType) {
127-
case variant_trait_v<const char*>:
128-
*reinterpret_cast<char**>(&(other.mStore)) = nullptr;
129-
return;
130-
case variant_trait_v<int*>:
131-
*reinterpret_cast<int**>(&(other.mStore)) = nullptr;
132-
return;
133-
case variant_trait_v<float*>:
134-
*reinterpret_cast<float**>(&(other.mStore)) = nullptr;
135-
return;
136-
case variant_trait_v<double*>:
137-
*reinterpret_cast<double**>(&(other.mStore)) = nullptr;
138-
return;
139-
case variant_trait_v<bool*>:
140-
*reinterpret_cast<bool**>(&(other.mStore)) = nullptr;
127+
case VariantType::String:
128+
case VariantType::ArrayInt:
129+
case VariantType::ArrayFloat:
130+
case VariantType::ArrayDouble:
131+
case VariantType::ArrayBool:
132+
case VariantType::ArrayString:
133+
*reinterpret_cast<void**>(&(other.mStore)) = nullptr;
141134
return;
142-
case variant_trait_v<std::string*>:
143-
*reinterpret_cast<std::string**>(&(other.mStore)) = nullptr;
144135
default:
145136
return;
146137
}
@@ -151,16 +142,20 @@ Variant::~Variant()
151142
// In case we allocated an array, we
152143
// should delete it.
153144
switch (mType) {
154-
case variant_trait_v<const char*>:
155-
case variant_trait_v<int*>:
156-
case variant_trait_v<float*>:
157-
case variant_trait_v<double*>:
158-
case variant_trait_v<bool*>:
159-
case variant_trait_v<std::string*>:
145+
case VariantType::String:
146+
case VariantType::ArrayInt:
147+
case VariantType::ArrayFloat:
148+
case VariantType::ArrayDouble:
149+
case VariantType::ArrayBool: {
160150
if (reinterpret_cast<void**>(&mStore) != nullptr) {
161151
free(*reinterpret_cast<void**>(&mStore));
162152
}
163153
return;
154+
}
155+
case VariantType::ArrayString: {
156+
// Allocated with placement new. Nothing to delete.
157+
return;
158+
}
164159
default:
165160
return;
166161
}
@@ -171,23 +166,23 @@ Variant& Variant::operator=(const Variant& other)
171166
mSize = other.mSize;
172167
mType = other.mType;
173168
switch (mType) {
174-
case variant_trait_v<const char*>:
169+
case VariantType::String:
175170
variant_helper<const char*>::set(&mStore, other.get<const char*>());
176171
return *this;
177-
case variant_trait_v<int*>:
172+
case VariantType::ArrayInt:
178173
variant_helper<int*>::set(&mStore, other.get<int*>(), mSize);
179174
return *this;
180-
case variant_trait_v<float*>:
175+
case VariantType::ArrayFloat:
181176
variant_helper<float*>::set(&mStore, other.get<float*>(), mSize);
182177
return *this;
183-
case variant_trait_v<double*>:
178+
case VariantType::ArrayDouble:
184179
variant_helper<double*>::set(&mStore, other.get<double*>(), mSize);
185180
return *this;
186-
case variant_trait_v<bool*>:
181+
case VariantType::ArrayBool:
187182
variant_helper<bool*>::set(&mStore, other.get<bool*>(), mSize);
188183
return *this;
189-
case variant_trait_v<std::string*>:
190-
variant_helper<std::string*>::set(&mStore, other.get<std::string*>(), mSize);
184+
case VariantType::ArrayString:
185+
variant_helper<std::vector<std::string>>::set(&mStore, other.get<std::vector<std::string>>());
191186
return *this;
192187
default:
193188
mStore = other.mStore;
@@ -200,29 +195,29 @@ Variant& Variant::operator=(Variant&& other) noexcept
200195
mSize = other.mSize;
201196
mType = other.mType;
202197
switch (mType) {
203-
case variant_trait_v<const char*>:
198+
case VariantType::String:
204199
variant_helper<const char*>::set(&mStore, other.get<const char*>());
205200
*reinterpret_cast<char**>(&(other.mStore)) = nullptr;
206201
return *this;
207-
case variant_trait_v<int*>:
202+
case VariantType::ArrayInt:
208203
variant_helper<int*>::set(&mStore, other.get<int*>(), mSize);
209204
*reinterpret_cast<int**>(&(other.mStore)) = nullptr;
210205
return *this;
211-
case variant_trait_v<float*>:
206+
case VariantType::ArrayFloat:
212207
variant_helper<float*>::set(&mStore, other.get<float*>(), mSize);
213208
*reinterpret_cast<float**>(&(other.mStore)) = nullptr;
214209
return *this;
215-
case variant_trait_v<double*>:
210+
case VariantType::ArrayDouble:
216211
variant_helper<double*>::set(&mStore, other.get<double*>(), mSize);
217212
*reinterpret_cast<double**>(&(other.mStore)) = nullptr;
218213
return *this;
219-
case variant_trait_v<bool*>:
214+
case VariantType::ArrayBool:
220215
variant_helper<bool*>::set(&mStore, other.get<bool*>(), mSize);
221216
*reinterpret_cast<bool**>(&(other.mStore)) = nullptr;
222217
return *this;
223-
case variant_trait_v<std::string*>:
224-
variant_helper<std::string*>::set(&mStore, other.get<std::string*>(), mSize);
225-
*reinterpret_cast<std::string**>(&(other.mStore)) = nullptr;
218+
case VariantType::ArrayString:
219+
variant_helper<std::vector<std::string>>::set(&mStore, other.get<std::vector<std::string>>());
220+
*reinterpret_cast<std::vector<std::string>**>(&(other.mStore)) = nullptr;
226221
return *this;
227222
default:
228223
mStore = other.mStore;

‎Framework/Core/src/VariantPropertyTreeHelpers.cxx‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,17 +19,17 @@ template boost::property_tree::ptree o2::framework::basicVectorToBranch(float*,
1919
template boost::property_tree::ptree o2::framework::basicVectorToBranch(int*, size_t);
2020
template boost::property_tree::ptree o2::framework::basicVectorToBranch(double*, size_t);
2121
template boost::property_tree::ptree o2::framework::basicVectorToBranch(bool*, size_t);
22-
template boost::property_tree::ptree o2::framework::basicVectorToBranch(std::basic_string<char>*, size_t);
22+
template boost::property_tree::ptree o2::framework::basicVectorToBranch(std::basic_string<char> const*, size_t);
2323

2424
template boost::property_tree::ptree o2::framework::vectorToBranch(std::vector<float>&& values);
2525
template boost::property_tree::ptree o2::framework::vectorToBranch(std::vector<int>&& values);
2626
template boost::property_tree::ptree o2::framework::vectorToBranch(std::vector<double>&& values);
2727
template boost::property_tree::ptree o2::framework::vectorToBranch(std::vector<std::string>&& values);
28-
template boost::property_tree::ptree o2::framework::vectorToBranch(float*, size_t);
29-
template boost::property_tree::ptree o2::framework::vectorToBranch(int*, size_t);
30-
template boost::property_tree::ptree o2::framework::vectorToBranch(double*, size_t);
31-
template boost::property_tree::ptree o2::framework::vectorToBranch(bool*, size_t);
32-
template boost::property_tree::ptree o2::framework::vectorToBranch(std::basic_string<char>*, size_t);
28+
template boost::property_tree::ptree o2::framework::vectorToBranch(float const*, size_t);
29+
template boost::property_tree::ptree o2::framework::vectorToBranch(int const*, size_t);
30+
template boost::property_tree::ptree o2::framework::vectorToBranch(double const*, size_t);
31+
template boost::property_tree::ptree o2::framework::vectorToBranch(bool const*, size_t);
32+
template boost::property_tree::ptree o2::framework::vectorToBranch(std::basic_string<char> const*, size_t);
3333

3434
template boost::property_tree::ptree o2::framework::labeledArrayToBranch(o2::framework::LabeledArray<float>&& array);
3535
template boost::property_tree::ptree o2::framework::labeledArrayToBranch(o2::framework::LabeledArray<int>&& array);

0 commit comments

Comments
 (0)