Skip to content

Commit a22360d

Browse files
cassavacassava
committed
fable: Use unique_ptr instead of raw pointers
1 parent 4dbb4a0 commit a22360d

File tree

9 files changed

+82
-26
lines changed

9 files changed

+82
-26
lines changed

‎fable/include/fable/schema.hpp

+4-6
Original file line numberDiff line numberDiff line change
@@ -200,11 +200,9 @@ class Schema : public schema::Interface {
200200

201201
// Interface
202202
template <typename T, std::enable_if_t<std::is_base_of_v<schema::Interface, T>, int> = 0>
203-
Schema(const T& value) : impl_(value.clone()) {} // NOLINT(runtime/explicit)
204-
Schema(schema::Interface* i) : impl_(i) { assert(impl_); } // NOLINT(runtime/explicit)
205-
Schema(std::shared_ptr<schema::Interface> i) : impl_(std::move(i)) { // NOLINT(runtime/explicit)
206-
assert(impl_);
207-
}
203+
Schema(const T& value) : impl_(value.clone()) {}
204+
Schema(std::unique_ptr<schema::Interface> ptr) : impl_(std::move(ptr)) { assert(impl_); }
205+
Schema(std::shared_ptr<schema::Interface> ptr) : impl_(std::move(ptr)) { assert(impl_); }
208206

209207
// Ignore
210208
Schema() : impl_(new schema::Ignore("")) {}
@@ -246,7 +244,7 @@ class Schema : public schema::Interface {
246244
public: // Overrides
247245
using Interface::to_json;
248246
[[nodiscard]] operator schema::Box() const { return schema::Box{impl_}; }
249-
[[nodiscard]] Interface* clone() const override { return impl_->clone(); }
247+
[[nodiscard]] std::unique_ptr<Interface> clone() const override { return impl_->clone(); }
250248
[[nodiscard]] JsonType type() const override { return impl_->type(); }
251249
[[nodiscard]] std::string type_string() const override { return impl_->type_string(); }
252250
[[nodiscard]] bool is_required() const override { return impl_->is_required(); }

‎fable/include/fable/schema/const.hpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ class Const : public Base<Const<T, P>> {
8787

8888
private:
8989
PrototypeSchema prototype_;
90-
const Type constant_;
90+
Type constant_;
9191
};
9292

9393
template <typename T, typename P, typename S>

‎fable/include/fable/schema/custom.hpp

+3-1
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,9 @@ class CustomDeserializer : public schema::Interface {
5454
}
5555

5656
public: // Overrides
57-
[[nodiscard]] Interface* clone() const override { return new CustomDeserializer(*this); }
57+
[[nodiscard]] std::unique_ptr<Interface> clone() const override {
58+
return std::make_unique<CustomDeserializer>(*this);
59+
}
5860

5961
using Interface::to_json;
6062
[[nodiscard]] JsonType type() const override { return impl_->type(); }

‎fable/include/fable/schema/factory.hpp

+62-10
Original file line numberDiff line numberDiff line change
@@ -57,18 +57,56 @@ class FactoryBase : public Base<CRTP> {
5757
public: // Types
5858
using Type = T;
5959
using MakeFunc = std::function<T(const Conf& c)>;
60+
61+
/**
62+
* TypeFactory is basically a pair with names for better readability.
63+
*/
6064
struct TypeFactory {
61-
TypeFactory(const Box& s, MakeFunc f) : schema(s), func(f) { schema.reset_ptr(); }
65+
TypeFactory(Box s, MakeFunc f) : schema(std::move(s)), func(std::move(f)) {
66+
schema.reset_ptr();
67+
}
6268

63-
Box schema;
64-
MakeFunc func;
69+
Box schema; // NOLINT
70+
MakeFunc func; // NOLINT
6571
};
6672

6773
using TransformFunc = std::function<Box(Struct&&)>;
6874
using FactoryMap = std::map<std::string, TypeFactory>;
6975
using FactoryPairList = std::initializer_list<std::pair<std::string, TypeFactory>>;
7076

7177
public: // Constructors
78+
~FactoryBase() noexcept override = default;
79+
80+
protected:
81+
FactoryBase(const FactoryBase& other)
82+
: Base<CRTP>(other)
83+
, transform_func_(other.transform_func_)
84+
, available_(other.available_)
85+
, factory_key_(other.factory_key_)
86+
, args_key_(other.args_key_)
87+
, args_subset_(other.args_subset_) {
88+
reset_schema();
89+
}
90+
91+
FactoryBase& operator=(const FactoryBase& other) {
92+
if (&other == this) {
93+
return *this;
94+
}
95+
Base<CRTP>::operator=(other);
96+
transform_func_ = other.transform_func_;
97+
available_ = other.available_;
98+
factory_key_ = other.factory_key_;
99+
args_key_ = other.args_key_;
100+
args_subset_ = other.args_subset_;
101+
reset_schema();
102+
return *this;
103+
}
104+
105+
FactoryBase(FactoryBase&&) noexcept = default;
106+
107+
FactoryBase& operator=(FactoryBase&&) noexcept = default;
108+
109+
public:
72110
/**
73111
* Construct an empty factory.
74112
*
@@ -137,7 +175,7 @@ class FactoryBase : public Base<CRTP> {
137175
* Common choices could be: factory, type, binding.
138176
*/
139177
void set_factory_key(const std::string& keyword) {
140-
assert(keyword != "");
178+
assert(!keyword.empty());
141179
factory_key_ = keyword;
142180
reset_schema();
143181
}
@@ -176,7 +214,7 @@ class FactoryBase : public Base<CRTP> {
176214
*
177215
* The default behavior is the identity function.
178216
*/
179-
void set_transform_schema(TransformFunc f) { transform_func_ = f; }
217+
void set_transform_schema(TransformFunc f) { transform_func_ = std::move(f); }
180218

181219
/**
182220
* Return the schema and factory function associated with the given key.
@@ -198,7 +236,7 @@ class FactoryBase : public Base<CRTP> {
198236
*/
199237
bool add_factory(const std::string& key, Box&& s, MakeFunc f) {
200238
if (!available_.count(key)) {
201-
available_.insert(std::make_pair(key, TypeFactory{std::move(s), f}));
239+
available_.insert(std::make_pair(key, TypeFactory{std::move(s), std::move(f)}));
202240
reset_schema();
203241
return true;
204242
}
@@ -212,7 +250,7 @@ class FactoryBase : public Base<CRTP> {
212250
if (!available_.count(key)) {
213251
available_.erase(key);
214252
}
215-
available_.insert(std::make_pair(key, TypeFactory{std::move(s), f}));
253+
available_.insert(std::make_pair(key, TypeFactory{std::move(s), std::move(f)}));
216254
reset_schema();
217255
}
218256

@@ -316,7 +354,7 @@ class FactoryBase : public Base<CRTP> {
316354
if (available_.size() == 0) {
317355
return;
318356
}
319-
schema_.reset(new Variant(factory_schemas()));
357+
schema_ = std::make_unique<Variant>(factory_schemas());
320358
}
321359

322360
[[nodiscard]] std::vector<Box> factory_schemas() const {
@@ -353,7 +391,7 @@ class FactoryBase : public Base<CRTP> {
353391
}
354392

355393
protected:
356-
std::shared_ptr<Variant> schema_;
394+
std::unique_ptr<Variant> schema_;
357395
TransformFunc transform_func_;
358396
FactoryMap available_;
359397
std::string factory_key_{"factory"};
@@ -366,7 +404,16 @@ class FactoryBase : public Base<CRTP> {
366404
* instance but is only usable through it's make() method.
367405
*/
368406
template <typename T>
369-
class FactoryPointerless : public FactoryBase<T, FactoryPointerless<T>> {};
407+
class FactoryPointerless : public FactoryBase<T, FactoryPointerless<T>> {
408+
public:
409+
using FactoryBase<T, FactoryPointerless<T>>::FactoryBase;
410+
FactoryPointerless() = default;
411+
FactoryPointerless(const FactoryPointerless<T>& other) = default;
412+
FactoryPointerless(FactoryPointerless<T>&& other) noexcept = default;
413+
FactoryPointerless<T>& operator=(const FactoryPointerless<T>& other) = default;
414+
FactoryPointerless<T>& operator=(FactoryPointerless<T>&& other) noexcept = default;
415+
~FactoryPointerless() override = default;
416+
};
370417

371418
/**
372419
* Factory is a factory schema that extends FactoryPointerless in that it
@@ -386,6 +433,11 @@ class Factory : public FactoryBase<T, Factory<T>> {
386433

387434
public: // Constructors
388435
using FactoryBase<T, Factory<T>>::FactoryBase;
436+
Factory(const Factory<T>& other) = default;
437+
Factory(Factory<T>&& other) noexcept = default;
438+
Factory<T>& operator=(const Factory<T>& other) = default;
439+
Factory<T>& operator=(Factory<T>&& other) noexcept = default;
440+
~Factory() override = default;
389441

390442
Factory(Type* ptr, std::string desc) : FactoryBase<T, Factory<T>>(std::move(desc)), ptr_(ptr) {}
391443

‎fable/include/fable/schema/interface.hpp

+6-4
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ class Interface {
7575
* This is implemented by Base and allows us to wrap implementors of
7676
* Interface with Schema.
7777
*/
78-
[[nodiscard]] virtual Interface* clone() const = 0;
78+
[[nodiscard]] virtual std::unique_ptr<Interface> clone() const = 0;
7979

8080
/**
8181
* Return whether the accepted input type is a variant.
@@ -295,7 +295,7 @@ class Box : public Interface {
295295
Box(Box&&) = default;
296296
Box& operator=(const Box&) = default;
297297

298-
Box(Interface* i) : impl_(i) { assert(impl_); } // NOLINT
298+
Box(std::unique_ptr<Interface> i) : impl_(std::move(i)) { assert(impl_); } // NOLINT
299299
Box(std::shared_ptr<Interface> i) : impl_(std::move(i)) { assert(impl_); } // NOLINT
300300

301301
public: // Special
@@ -357,7 +357,7 @@ class Box : public Interface {
357357

358358
public: // Overrides
359359
using Interface::to_json;
360-
[[nodiscard]] Interface* clone() const override { return impl_->clone(); }
360+
[[nodiscard]] std::unique_ptr<Interface> clone() const override { return impl_->clone(); }
361361
[[nodiscard]] JsonType type() const override { return impl_->type(); }
362362
[[nodiscard]] std::string type_string() const override { return impl_->type_string(); }
363363
[[nodiscard]] bool is_required() const override { return impl_->is_required(); }
@@ -395,7 +395,9 @@ class Base : public Interface {
395395
explicit Base(std::string desc) : desc_(std::move(desc)) {}
396396
virtual ~Base() = default;
397397

398-
[[nodiscard]] Interface* clone() const override { return new CRTP(static_cast<CRTP const&>(*this)); }
398+
[[nodiscard]] std::unique_ptr<Interface> clone() const override {
399+
return std::make_unique<CRTP>(static_cast<CRTP const&>(*this));
400+
}
399401
[[nodiscard]] operator Box() const { return Box{this->clone()}; }
400402

401403
[[nodiscard]] JsonType type() const override { return type_; }

‎fable/include/fable/schema/struct.hpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,7 @@ class Struct : public Base<Struct> {
184184
template <typename S, typename = enable_if_schema_t<S>>
185185
void set_additional_properties(const S& s) {
186186
additional_properties_ = true;
187-
additional_prototype_.reset(s.clone());
187+
additional_prototype_ = s.clone();
188188
additional_prototype_->reset_ptr();
189189
}
190190

‎fable/include/fable/schema/variant.hpp

+3-1
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,9 @@ class Variant : public Interface {
6363
Variant(std::string desc, std::vector<Box>&& vec);
6464

6565
public: // Base
66-
[[nodiscard]] Interface* clone() const override { return new Variant(*this); }
66+
[[nodiscard]] std::unique_ptr<Interface> clone() const override {
67+
return std::make_unique<Variant>(*this);
68+
}
6769
[[nodiscard]] operator Box() const { return Box{this->clone()}; }
6870
[[nodiscard]] JsonType type() const override { return type_; }
6971
[[nodiscard]] std::string type_string() const override { return type_string_; }

‎fable/src/fable/confable.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ Schema& Confable::schema() {
5656
if (!schema_) {
5757
schema_ = std::make_unique<Schema>(schema_impl());
5858
}
59-
return *schema_.get();
59+
return *schema_;
6060
}
6161

6262
const Schema& Confable::schema() const { return const_cast<Confable*>(this)->schema(); }

‎fable/src/fable/schema/factory_test.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,7 @@ struct MyFactoryStructWithoutArgs : public fable::Confable {
198198
{
199199
"prime", {
200200
Struct{
201-
{"n", fable::make_prototype<uint8_t>().bounds_with(1,6, {}).require()},
201+
{"n", fable::make_prototype<uint8_t>().bounds_with(1, 6, {}).require()},
202202
},
203203
[](const fable::Conf& c) -> int {
204204
static const std::vector<int> primes{2, 3, 5, 7, 11, 13};

0 commit comments

Comments
 (0)