From: Daniel Elstner <redacted>
Date: Thu, 15 Oct 2015 16:32:44 +0000 (+0200)
Subject: C++: Use smart pointers instead of manual delete
X-Git-Tag: libsigrok-0.4.0~168
X-Git-Url: http://sigrok.org/gitweb/?a=commitdiff_plain;h=f17b45465500f1d1aad58479802018949004cce5;hp=d5d7b09eb76de71a42b780b97aee5f5019d2d799;p=libsigrok.git

C++: Use smart pointers instead of manual delete

Make use of std::unique_ptr<> to manage the lifetime of members
or container elements, so that manual invocations of delete can
be avoided. This also provides for exception safety.

Since std::unique_ptr<> is only movable but not copyable, adapt
the code to avoid copies and assignments of these pointers.
---

diff --git a/bindings/cxx/classes.cpp b/bindings/cxx/classes.cpp
index 7f5b3b24..5a977a86 100644
--- a/bindings/cxx/classes.cpp
+++ b/bindings/cxx/classes.cpp
@@ -47,9 +47,9 @@ static inline const char *valid_string(const char *input)
 /** Helper function to convert between map<string, VariantBase> and GHashTable */
 static GHashTable *map_to_hash_variant(const map<string, Glib::VariantBase> &input)
 {
-	auto output = g_hash_table_new_full(g_str_hash, g_str_equal, g_free,
+	auto *const output = g_hash_table_new_full(g_str_hash, g_str_equal, g_free,
 			reinterpret_cast<GDestroyNotify>(&g_variant_unref));
-	for (auto entry : input)
+	for (const auto &entry : input)
 		g_hash_table_insert(output,
 			g_strdup(entry.first.c_str()),
 			entry.second.gobj_copy());
@@ -125,21 +125,23 @@ Context::Context() :
 {
 	check(sr_init(&_structure));
 
-	struct sr_dev_driver **driver_list = sr_driver_list(_structure);
-	if (driver_list)
-		for (int i = 0; driver_list[i]; i++)
-			_drivers[driver_list[i]->name] =
-				new Driver(driver_list[i]);
-	const struct sr_input_module **input_list = sr_input_list();
-	if (input_list)
-		for (int i = 0; input_list[i]; i++)
-			_input_formats[sr_input_id_get(input_list[i])] =
-				new InputFormat(input_list[i]);
-	const struct sr_output_module **output_list = sr_output_list();
-	if (output_list)
-		for (int i = 0; output_list[i]; i++)
-			_output_formats[sr_output_id_get(output_list[i])] =
-				new OutputFormat(output_list[i]);
+	if (struct sr_dev_driver **driver_list = sr_driver_list(_structure))
+		for (int i = 0; driver_list[i]; i++) {
+			unique_ptr<Driver> driver {new Driver{driver_list[i]}};
+			_drivers.emplace(driver->name(), move(driver));
+		}
+
+	if (const struct sr_input_module **input_list = sr_input_list())
+		for (int i = 0; input_list[i]; i++) {
+			unique_ptr<InputFormat> input {new InputFormat{input_list[i]}};
+			_input_formats.emplace(input->name(), move(input));
+		}
+
+	if (const struct sr_output_module **output_list = sr_output_list())
+		for (int i = 0; output_list[i]; i++) {
+			unique_ptr<OutputFormat> output {new OutputFormat{output_list[i]}};
+			_output_formats.emplace(output->name(), move(output));
+		}
 }
 
 string Context::package_version()
@@ -155,11 +157,11 @@ string Context::lib_version()
 map<string, shared_ptr<Driver>> Context::drivers()
 {
 	map<string, shared_ptr<Driver>> result;
-	for (auto entry: _drivers)
+	for (const auto &entry: _drivers)
 	{
-		auto name = entry.first;
-		auto driver = entry.second;
-		result[name] = driver->share_owned_by(shared_from_this());
+		const auto &name = entry.first;
+		const auto &driver = entry.second;
+		result.emplace(name, driver->share_owned_by(shared_from_this()));
 	}
 	return result;
 }
@@ -167,11 +169,11 @@ map<string, shared_ptr<Driver>> Context::drivers()
 map<string, shared_ptr<InputFormat>> Context::input_formats()
 {
 	map<string, shared_ptr<InputFormat>> result;
-	for (auto entry: _input_formats)
+	for (const auto &entry: _input_formats)
 	{
-		auto name = entry.first;
-		auto input_format = entry.second;
-		result[name] = input_format->share_owned_by(shared_from_this());
+		const auto &name = entry.first;
+		const auto &input_format = entry.second;
+		result.emplace(name, input_format->share_owned_by(shared_from_this()));
 	}
 	return result;
 }
@@ -179,23 +181,17 @@ map<string, shared_ptr<InputFormat>> Context::input_formats()
 map<string, shared_ptr<OutputFormat>> Context::output_formats()
 {
 	map<string, shared_ptr<OutputFormat>> result;
-	for (auto entry: _output_formats)
+	for (const auto &entry: _output_formats)
 	{
-		auto name = entry.first;
-		auto output_format = entry.second;
-		result[name] = output_format->share_owned_by(shared_from_this());
+		const auto &name = entry.first;
+		const auto &output_format = entry.second;
+		result.emplace(name, output_format->share_owned_by(shared_from_this()));
 	}
 	return result;
 }
 
 Context::~Context()
 {
-	for (auto entry : _drivers)
-		delete entry.second;
-	for (auto entry : _input_formats)
-		delete entry.second;
-	for (auto entry : _output_formats)
-		delete entry.second;
 	check(sr_exit(_structure));
 }
 
@@ -283,14 +279,13 @@ shared_ptr<Packet> Context::create_meta_packet(
 	const map<const ConfigKey *, Glib::VariantBase> &config)
 {
 	auto meta = g_new0(struct sr_datafeed_meta, 1);
-	for (auto input : config)
+	for (const auto &input : config)
 	{
-		auto key = input.first;
-		auto value = input.second;
-		auto output = g_new(struct sr_config, 1);
+		const auto &key = input.first;
+		const auto &value = input.second;
+		auto *const output = g_new(struct sr_config, 1);
 		output->key = key->id();
-		output->data = value.gobj();
-		g_variant_ref(output->data);
+		output->data = value.gobj_copy();
 		meta->config = g_slist_append(meta->config, output);
 	}
 	auto packet = g_new(struct sr_datafeed_packet, 1);
@@ -322,7 +317,7 @@ shared_ptr<Packet> Context::create_analog_packet(
 
 	analog->meaning = meaning;
 
-	for (auto channel : channels)
+	for (const auto &channel : channels)
 		meaning->channels = g_slist_append(meaning->channels, channel->_structure);
 	analog->num_samples = num_samples;
 	meaning->mq = static_cast<sr_mq>(mq->id());
@@ -416,13 +411,13 @@ vector<shared_ptr<HardwareDevice>> Driver::scan(
 
 	/* Translate scan options to GSList of struct sr_config pointers. */
 	GSList *option_list = nullptr;
-	for (auto entry : options)
+	for (const auto &entry : options)
 	{
-		auto key = entry.first;
-		auto value = entry.second;
-		auto config = g_new(struct sr_config, 1);
+		const auto &key = entry.first;
+		const auto &value = entry.second;
+		auto *const config = g_new(struct sr_config, 1);
 		config->key = key->id();
-		config->data = value.gobj();
+		config->data = const_cast<GVariant*>(value.gobj());
 		option_list = g_slist_append(option_list, config);
 	}
 
@@ -554,24 +549,21 @@ Device::Device(struct sr_dev_inst *structure) :
 {
 	for (GSList *entry = sr_dev_inst_channels_get(structure); entry; entry = entry->next)
 	{
-		auto *const channel = static_cast<struct sr_channel *>(entry->data);
-		_channels[channel] = new Channel(channel);
+		auto *const ch = static_cast<struct sr_channel *>(entry->data);
+		unique_ptr<Channel> channel {new Channel{ch}};
+		_channels.emplace(ch, move(channel));
 	}
 
 	for (GSList *entry = sr_dev_inst_channel_groups_get(structure); entry; entry = entry->next)
 	{
-		auto *const group = static_cast<struct sr_channel_group *>(entry->data);
-		_channel_groups[group->name] = new ChannelGroup(this, group);
+		auto *const cg = static_cast<struct sr_channel_group *>(entry->data);
+		unique_ptr<ChannelGroup> group {new ChannelGroup{this, cg}};
+		_channel_groups.emplace(group->name(), move(group));
 	}
 }
 
 Device::~Device()
-{
-	for (auto entry : _channels)
-		delete entry.second;
-	for (auto entry : _channel_groups)
-		delete entry.second;
-}
+{}
 
 string Device::vendor() const
 {
@@ -617,11 +609,11 @@ map<string, shared_ptr<ChannelGroup>>
 Device::channel_groups()
 {
 	map<string, shared_ptr<ChannelGroup>> result;
-	for (auto entry: _channel_groups)
+	for (const auto &entry: _channel_groups)
 	{
-		auto name = entry.first;
-		auto channel_group = entry.second;
-		result[name] = channel_group->share_owned_by(get_shared_from_this());
+		const auto &name = entry.first;
+		const auto &channel_group = entry.second;
+		result.emplace(name, channel_group->share_owned_by(get_shared_from_this()));
 	}
 	return result;
 }
@@ -679,7 +671,8 @@ shared_ptr<Channel> UserDevice::add_channel(unsigned int index,
 		index, type->id(), name.c_str()));
 	GSList *const last = g_slist_last(sr_dev_inst_channels_get(Device::_structure));
 	auto *const ch = static_cast<struct sr_channel *>(last->data);
-	_channels[ch] = new Channel(ch);
+	unique_ptr<Channel> channel {new Channel{ch}};
+	_channels.emplace(ch, move(channel));
 	return get_channel(ch);
 }
 
@@ -723,13 +716,15 @@ unsigned int Channel::index() const
 	return _structure->index;
 }
 
-ChannelGroup::ChannelGroup(Device *device,
+ChannelGroup::ChannelGroup(const Device *device,
 		struct sr_channel_group *structure) :
 	Configurable(sr_dev_inst_driver_get(device->_structure), device->_structure, structure)
 {
 	for (GSList *entry = config_channel_group->channels; entry; entry = entry->next) {
 		auto *const ch = static_cast<struct sr_channel *>(entry->data);
-		_channels.push_back(device->_channels[ch]);
+		/* Note: This relies on Device::_channels to keep the Channel
+		 * objects around over the lifetime of the ChannelGroup. */
+		_channels.push_back(device->_channels.find(ch)->second.get());
 	}
 }
 
@@ -745,7 +740,7 @@ string ChannelGroup::name() const
 vector<shared_ptr<Channel>> ChannelGroup::channels()
 {
 	vector<shared_ptr<Channel>> result;
-	for (auto channel : _channels)
+	for (const auto &channel : _channels)
 		result.push_back(channel->share_owned_by(_parent));
 	return result;
 }
@@ -754,16 +749,15 @@ Trigger::Trigger(shared_ptr<Context> context, string name) :
 	_structure(sr_trigger_new(name.c_str())),
 	_context(move(context))
 {
-	for (auto stage = _structure->stages; stage; stage = stage->next)
-		_stages.push_back(
-			new TriggerStage(static_cast<struct sr_trigger_stage *>(stage->data)));
+	for (auto *stage = _structure->stages; stage; stage = stage->next) {
+		unique_ptr<TriggerStage> ts {new TriggerStage{
+				static_cast<struct sr_trigger_stage *>(stage->data)}};
+		_stages.push_back(move(ts));
+	}
 }
 
 Trigger::~Trigger()
 {
-	for (auto stage: _stages)
-		delete stage;
-
 	sr_trigger_free(_structure);
 }
 
@@ -775,16 +769,16 @@ string Trigger::name() const
 vector<shared_ptr<TriggerStage>> Trigger::stages()
 {
 	vector<shared_ptr<TriggerStage>> result;
-	for (auto stage : _stages)
+	for (const auto &stage : _stages)
 		result.push_back(stage->share_owned_by(shared_from_this()));
 	return result;
 }
 
 shared_ptr<TriggerStage> Trigger::add_stage()
 {
-	auto stage = new TriggerStage(sr_trigger_stage_add(_structure));
-	_stages.push_back(stage);
-	return stage->share_owned_by(shared_from_this());
+	unique_ptr<TriggerStage> stage {new TriggerStage{sr_trigger_stage_add(_structure)}};
+	_stages.push_back(move(stage));
+	return _stages.back()->share_owned_by(shared_from_this());
 }
 
 TriggerStage::TriggerStage(struct sr_trigger_stage *structure) :
@@ -794,8 +788,6 @@ TriggerStage::TriggerStage(struct sr_trigger_stage *structure) :
 
 TriggerStage::~TriggerStage()
 {
-	for (auto match : _matches)
-		delete match;
 }
 	
 int TriggerStage::number() const
@@ -806,7 +798,7 @@ int TriggerStage::number() const
 vector<shared_ptr<TriggerMatch>> TriggerStage::matches()
 {
 	vector<shared_ptr<TriggerMatch>> result;
-	for (auto match : _matches)
+	for (const auto &match : _matches)
 		result.push_back(match->share_owned_by(shared_from_this()));
 	return result;
 }
@@ -817,9 +809,10 @@ void TriggerStage::add_match(shared_ptr<Channel> channel,
 	check(sr_trigger_match_add(_structure,
 		channel->_structure, type->id(), value));
 	GSList *const last = g_slist_last(_structure->matches);
-	_matches.push_back(new TriggerMatch(
-		static_cast<struct sr_trigger_match *>(last->data),
-		move(channel)));
+	unique_ptr<TriggerMatch> match {new TriggerMatch{
+			static_cast<struct sr_trigger_match *>(last->data),
+			move(channel)}};
+	_matches.push_back(move(match));
 }
 
 void TriggerStage::add_match(shared_ptr<Channel> channel,
@@ -901,7 +894,8 @@ Session::Session(shared_ptr<Context> context, string filename) :
 	check(sr_session_dev_list(_structure, &dev_list));
 	for (GSList *dev = dev_list; dev; dev = dev->next) {
 		auto *const sdi = static_cast<struct sr_dev_inst *>(dev->data);
-		_owned_devices[sdi] = new SessionDevice(sdi);
+		unique_ptr<SessionDevice> device {new SessionDevice{sdi}};
+		_owned_devices.emplace(sdi, move(device));
 	}
 	_context->_session = this;
 }
@@ -909,12 +903,6 @@ Session::Session(shared_ptr<Context> context, string filename) :
 Session::~Session()
 {
 	check(sr_session_destroy(_structure));
-
-	for (auto callback : _datafeed_callbacks)
-		delete callback;
-
-	for (auto entry : _owned_devices)
-		delete entry.second;
 }
 
 shared_ptr<Device> Session::get_device(const struct sr_dev_inst *sdi)
@@ -1002,17 +990,16 @@ static void datafeed_callback(const struct sr_dev_inst *sdi,
 
 void Session::add_datafeed_callback(DatafeedCallbackFunction callback)
 {
-	auto cb_data = new DatafeedCallbackData(this, move(callback));
+	unique_ptr<DatafeedCallbackData> cb_data
+		{new DatafeedCallbackData{this, move(callback)}};
 	check(sr_session_datafeed_callback_add(_structure,
-		datafeed_callback, cb_data));
-	_datafeed_callbacks.push_back(cb_data);
+			&datafeed_callback, cb_data.get()));
+	_datafeed_callbacks.push_back(move(cb_data));
 }
 
 void Session::remove_datafeed_callbacks()
 {
 	check(sr_session_datafeed_callback_remove_all(_structure));
-	for (auto callback : _datafeed_callbacks)
-		delete callback;
 	_datafeed_callbacks.clear();
 }
 
@@ -1049,35 +1036,30 @@ Packet::Packet(shared_ptr<Device> device,
 	switch (structure->type)
 	{
 		case SR_DF_HEADER:
-			_payload = new Header(
+			_payload.reset(new Header{
 				static_cast<const struct sr_datafeed_header *>(
-					structure->payload));
+					structure->payload)});
 			break;
 		case SR_DF_META:
-			_payload = new Meta(
+			_payload.reset(new Meta{
 				static_cast<const struct sr_datafeed_meta *>(
-					structure->payload));
+					structure->payload)});
 			break;
 		case SR_DF_LOGIC:
-			_payload = new Logic(
+			_payload.reset(new Logic{
 				static_cast<const struct sr_datafeed_logic *>(
-					structure->payload));
+					structure->payload)});
 			break;
 		case SR_DF_ANALOG:
-			_payload = new Analog(
+			_payload.reset(new Analog{
 				static_cast<const struct sr_datafeed_analog *>(
-					structure->payload));
-			break;
-		default:
-			_payload = nullptr;
+					structure->payload)});
 			break;
 	}
 }
 
 Packet::~Packet()
 {
-	if (_payload)
-		delete _payload;
 }
 
 const PacketType *Packet::type() const
@@ -1291,8 +1273,7 @@ shared_ptr<Input> InputFormat::create_input(
 
 Input::Input(shared_ptr<Context> context, const struct sr_input *structure) :
 	_structure(structure),
-	_context(move(context)),
-	_device(nullptr)
+	_context(move(context))
 {
 }
 
@@ -1303,7 +1284,7 @@ shared_ptr<InputDevice> Input::device()
 		auto sdi = sr_input_dev_inst_get(_structure);
 		if (!sdi)
 			throw Error(SR_ERR_NA);
-		_device = new InputDevice(shared_from_this(), sdi);
+		_device.reset(new InputDevice{shared_from_this(), sdi});
 	}
 
 	return _device->share_owned_by(shared_from_this());
@@ -1324,8 +1305,6 @@ void Input::end()
 
 Input::~Input()
 {
-	if (_device)
-		delete _device;
 	sr_input_free(_structure);
 }
 
diff --git a/bindings/cxx/include/libsigrokcxx/libsigrokcxx.hpp b/bindings/cxx/include/libsigrokcxx/libsigrokcxx.hpp
index c7995b8c..5e101fc8 100644
--- a/bindings/cxx/include/libsigrokcxx/libsigrokcxx.hpp
+++ b/bindings/cxx/include/libsigrokcxx/libsigrokcxx.hpp
@@ -300,9 +300,9 @@ public:
 	map<string, string> serials(shared_ptr<Driver> driver) const;
 private:
 	struct sr_context *_structure;
-	map<string, Driver *> _drivers;
-	map<string, InputFormat *> _input_formats;
-	map<string, OutputFormat *> _output_formats;
+	map<string, unique_ptr<Driver> > _drivers;
+	map<string, unique_ptr<InputFormat> > _input_formats;
+	map<string, unique_ptr<OutputFormat> > _output_formats;
 	Session *_session;
 	LogCallbackFunction _log_callback;
 	Context();
@@ -370,6 +370,7 @@ private:
 	friend class Context;
 	friend class HardwareDevice;
 	friend class ChannelGroup;
+	friend class std::default_delete<Driver>;
 };
 
 /** A generic device, either hardware or virtual */
@@ -401,9 +402,9 @@ protected:
 	shared_ptr<Channel> get_channel(struct sr_channel *ptr);
 
 	struct sr_dev_inst *_structure;
-	map<struct sr_channel *, Channel *> _channels;
+	map<struct sr_channel *, unique_ptr<Channel> > _channels;
 private:
-	map<string, ChannelGroup *> _channel_groups;
+	map<string, unique_ptr<ChannelGroup> > _channel_groups;
 	/** Deleter needed to allow shared_ptr use with protected destructor. */
 	class Deleter
 	{
@@ -494,6 +495,7 @@ private:
 	friend class Session;
 	friend class TriggerStage;
 	friend class Context;
+	friend class std::default_delete<Channel>;
 };
 
 /** A group of channels on a device, which share some configuration */
@@ -507,10 +509,11 @@ public:
 	/** List of the channels in this group. */
 	vector<shared_ptr<Channel> > channels();
 private:
-	ChannelGroup(Device *device, struct sr_channel_group *structure);
+	ChannelGroup(const Device *device, struct sr_channel_group *structure);
 	~ChannelGroup();
 	vector<Channel *> _channels;
 	friend class Device;
+	friend class std::default_delete<ChannelGroup>;
 };
 
 /** A trigger configuration */
@@ -528,7 +531,7 @@ private:
 	~Trigger();
 	struct sr_trigger *_structure;
 	shared_ptr<Context> _context;
-	vector<TriggerStage *> _stages;
+	vector<unique_ptr<TriggerStage> > _stages;
 	friend class Deleter;
 	friend class Context;
 	friend class Session;
@@ -554,10 +557,11 @@ public:
 	void add_match(shared_ptr<Channel> channel, const TriggerMatchType *type, float value);
 private:
 	struct sr_trigger_stage *_structure;
-	vector<TriggerMatch *> _matches;
+	vector<unique_ptr<TriggerMatch> > _matches;
 	explicit TriggerStage(struct sr_trigger_stage *structure);
 	~TriggerStage();
 	friend class Trigger;
+	friend class std::default_delete<TriggerStage>;
 };
 
 /** A match condition in a trigger configuration  */
@@ -577,6 +581,7 @@ private:
 	struct sr_trigger_match *_structure;
 	shared_ptr<Channel> _channel;
 	friend class TriggerStage;
+	friend class std::default_delete<TriggerMatch>;
 };
 
 /** Type of session stopped callback */
@@ -617,6 +622,7 @@ private:
 	};
 	friend class Deleter;
 	friend class Session;
+	friend class std::default_delete<SessionDevice>;
 };
 
 /** A sigrok session */
@@ -661,9 +667,9 @@ private:
 	shared_ptr<Device> get_device(const struct sr_dev_inst *sdi);
 	struct sr_session *_structure;
 	const shared_ptr<Context> _context;
-	map<const struct sr_dev_inst *, SessionDevice *> _owned_devices;
+	map<const struct sr_dev_inst *, unique_ptr<SessionDevice> > _owned_devices;
 	map<const struct sr_dev_inst *, shared_ptr<Device> > _other_devices;
-	vector<DatafeedCallbackData *> _datafeed_callbacks;
+	vector<unique_ptr<DatafeedCallbackData> > _datafeed_callbacks;
 	SessionStoppedCallback _stopped_callback;
 	string _filename;
 	shared_ptr<Trigger> _trigger;
@@ -687,7 +693,7 @@ private:
 	~Packet();
 	const struct sr_datafeed_packet *_structure;
 	shared_ptr<Device> _device;
-	PacketPayload *_payload;
+	unique_ptr<PacketPayload> _payload;
 	friend class Deleter;
 	friend class Session;
 	friend class Output;
@@ -717,6 +723,7 @@ private:
 	friend class Deleter;
 	friend class Packet;
 	friend class Output;
+	friend class std::default_delete<PacketPayload>;
 };
 
 /** Payload of a datafeed header packet */
@@ -833,6 +840,7 @@ private:
 
 	friend class Context;
 	friend class InputDevice;
+	friend class std::default_delete<InputFormat>;
 };
 
 /** An input instance (an input format applied to a file or stream) */
@@ -852,7 +860,7 @@ private:
 	~Input();
 	const struct sr_input *_structure;
 	shared_ptr<Context> _context;
-	InputDevice *_device;
+	unique_ptr<InputDevice> _device;
 	friend class Deleter;
 	friend class Context;
 	friend class InputFormat;
@@ -869,6 +877,7 @@ private:
 	shared_ptr<Device> get_shared_from_this();
 	shared_ptr<Input> _input;
 	friend class Input;
+	friend class std::default_delete<InputDevice>;
 };
 
 /** An option used by an output format */
@@ -938,6 +947,7 @@ private:
 
 	friend class Context;
 	friend class Output;
+	friend class std::default_delete<OutputFormat>;
 };
 
 /** An output instance (an output format applied to a device) */