several fixes to move semantics

code/controlconfig/controlsconfig.cpp

@@ -739,7 +739,7 @@ void control_config_bind(int i, const CC_bind &new_bind, selItem order, bool API
 	Undo_stack stack;
 	stack.save(Control_config[i].first);
 	stack.save(Control_config[i].second);
-	Undo_controls.save_stack(stack);
+	Undo_controls.save_stack(std::move(stack));
 
 	CCB old(Control_config[i]);
 
@@ -770,7 +770,7 @@ bool control_config_remove_binding(int ctrl, selItem item, bool API_Access)
 
 			stack.save(Control_config[ctrl].first);
 			stack.save(Control_config[ctrl].second);
-			Undo_controls.save_stack(stack);
+			Undo_controls.save_stack(std::move(stack));
 
 			Control_config[ctrl].first.clear();
 			Control_config[ctrl].second.clear();
@@ -873,7 +873,7 @@ bool control_config_clear_other(int ctrl, bool API_Access)
 		return false;
 	}
 
-	Undo_controls.save_stack(stack);
+	Undo_controls.save_stack(std::move(stack));
 	control_config_conflict_check();
 
 	if (!API_Access) {
@@ -911,7 +911,7 @@ bool control_config_clear_all(bool API_Access)
 		return false;
 	}
 
-	Undo_controls.save_stack(stack);
+	Undo_controls.save_stack(std::move(stack));
 	control_config_conflict_check();
 
 	if (!API_Access) {
@@ -975,7 +975,7 @@ bool control_config_do_reset(bool cycle, bool API_Access)
 		stack.save(item.first);
 		stack.save(item.second);
 	}
-	Undo_controls.save_stack(stack);
+	Undo_controls.save_stack(std::move(stack));
 
 	control_config_use_preset(Control_config_presets[Defaults_cycle_pos]);
 
@@ -1097,7 +1097,7 @@ bool control_config_toggle_modifier(int bit, int ctrl, bool API_Access)
 	Undo_stack stack;
 	stack.save(Control_config[ctrl].first);
 	stack.save(Control_config[ctrl].second);
-	Undo_controls.save_stack(stack);
+	Undo_controls.save_stack(std::move(stack));
 
 	Control_config[ctrl].take(CC_bind(CID_KEYBOARD, static_cast<short>(k ^ bit)), -1);
 	control_config_conflict_check();

code/decals/decals.h

@@ -5,6 +5,7 @@
 #include "object/object.h"
 
 #include "utils/RandomRange.h"
+#include "utils/reset_on_move.h"
 
 namespace decals {
 
@@ -20,9 +21,12 @@ class DecalDefinition {
 	SCP_string _normalMapFilename;
 	bool _loopNormal = true;
 
-	int _diffuseBitmap = -1;
-	int _glowBitmap = -1;
-	int _normalBitmap = -1;
+	// reset_on_move so that the defaulted move operations below transfer the
+	// handles; the destructor bm_release()s them, so a moved-from element must
+	// not keep aliasing live handles
+	util::reset_on_move<int, -1> _diffuseBitmap;
+	util::reset_on_move<int, -1> _glowBitmap;
+	util::reset_on_move<int, -1> _normalBitmap;
 
  public:
 	explicit DecalDefinition(SCP_string name);

code/globalincs/undosys.cpp

@@ -6,6 +6,8 @@
 #include "globalincs/undosys.h"
 #include "globalincs/vmallocator.h"
 
+#include <utility>
+
 // Pure virtual deconstructors must be defined.
 Undo_item_base::~Undo_item_base() = default;
 
@@ -47,22 +49,20 @@ void Undo_system::clear_redo() {
 	redo_stack.clear();
 }
 
-size_t Undo_system::save_stack(Undo_stack& stack) {
+size_t Undo_system::save_stack(Undo_stack&& stack) {
 	if (stack.size() == 0) {
 		return 0;
 	}
-	
+
 	clear_redo();
 
-	// Copy the stack onto the heap, and push it into our undo_stack as a single item
-	Undo_item_base *new_item = new Undo_stack(stack);
+	// Move the stack onto the heap, and push it into our undo_stack as a single item.
+	// The move empties the input stack, so there's only one deletion handler for the items.
+	Undo_item_base *new_item = new Undo_stack(std::move(stack));
 
 	Assert(new_item != nullptr);
 	undo_stack.push_back(new_item);
 
-	// Input stack is told to untrack the items, so that there's only one deletion handler for them
-	stack.untrack();
-
 	clamp_stacks();
 
 	return undo_stack.size();
@@ -129,6 +129,18 @@ Undo_stack::~Undo_stack() {
 	clear();
 }
 
+Undo_stack& Undo_stack::operator=(Undo_stack&& other) noexcept {
+	if (this != &other) {
+		// delete any items we currently track, since the vector assignment below would otherwise leak them
+		clear();
+
+		reverse = other.reverse;
+		// exchange rather than move, to guarantee the source is left empty and never deletes the items
+		stack = std::exchange(other.stack, {});
+	}
+	return *this;
+}
+
 void Undo_stack::reserve(size_t size) {
 	stack.reserve(size);
 }
@@ -154,10 +166,6 @@ size_t Undo_stack::size() {
 	return stack.size();
 }
 
-void Undo_stack::untrack() {
-	stack.clear();
-}
-
 void Undo_stack::clear() {
 	for (auto &element : stack) {
 		delete element;

code/globalincs/undosys.h

@@ -53,7 +53,7 @@ Lastly, you can make stacks of undo operations, so that a single op in the syste
 		for (int i = 0; i < JOY_AXIS_ACTIONS; ++i) {
 			stack.save(Axis_map_to[i], Axis_map_to);		// This example saves a C style array. Does the same thing without a wrapper, but wasteful
 
-		Undo_controls.save_stack(stack) // Save the stack as a single item!
+		Undo_controls.save_stack(std::move(stack)) // Save the stack as a single item!  (the stack is empty afterwards)
 	}
 
 	std::pair<const void*, const void*> ref = Undo_controls.undo();
@@ -161,6 +161,14 @@ class Undo_stack : public Undo_item_base
 
 	~Undo_stack() override;
 
+	// The destructor deletes the tracked items, so a copy would double-delete;
+	// a stack is transferred into the undo system by move, which empties the
+	// source so that only one owner ever deletes the items.
+	Undo_stack(const Undo_stack&) = delete;
+	Undo_stack& operator=(const Undo_stack&) = delete;
+	Undo_stack(Undo_stack&&) noexcept = default;
+	Undo_stack& operator=(Undo_stack&&) noexcept;
+
 	/*!
 	 * @brief Restores all items within the undo stack
 	 *
@@ -202,15 +210,6 @@ class Undo_stack : public Undo_item_base
 	  */
 	 void clear();
 
-protected:
-	friend class Undo_system;
-
-	/*!
-	 * @brief Calls ::clear() on the internal vector
-	 * @note Caution: This does not delete the tracked Undo_items
-	 */
-	void untrack();
-
 private:
 	bool reverse;   // Direction to walk the stack. forward = false, reverse = True
 
@@ -259,13 +258,14 @@ class Undo_system
 	/*!
 	 * @brief Saves a stack of undo-items as a single undo-item within the system
 	 *
-	 * @param[in,out] stack  The undo stack to save to the undo system.  Data in the stack is "unspecified" after this operation
+	 * @param[in,out] stack  The undo stack to save to the undo system.  Must be passed with std::move(); the
+	 *  stack is empty after this operation
 	 *
-	 * @details  The undo system effectively moves the stack into its internal containers, claiming ownership of the
-	 *  Undo_items and deleting them upon going out of scope.  The input stack is told to untrack the Undo_items in
-	 *  the process, so that there is only ever one reference to the Undo_item like a std::unique_ptr
+	 * @details  The undo system moves the stack into its internal containers, claiming ownership of the
+	 *  Undo_items and deleting them upon going out of scope.  The move empties the input stack, so that there
+	 *  is only ever one owner of each Undo_item, like a std::unique_ptr
 	 */
-	size_t save_stack(Undo_stack& stack);
+	size_t save_stack(Undo_stack&& stack);
 
 	/*!
 	 * @brief Undo's the last changed item and save the changes into the Redo stack

code/graphics/grbatch.cpp

@@ -13,6 +13,8 @@
 #include "graphics/grbatch.h"
 #include "render/3d.h"
 
+#include <utility>
+
 geometry_batcher::~geometry_batcher()
 {
 	if (vert != NULL) {
@@ -124,6 +126,15 @@ void geometry_batcher::clone(const geometry_batcher &geo)
 	n_to_render = geo.n_to_render;
 	n_allocated = geo.n_allocated;
 	use_radius = geo.use_radius;
+	buffer_offset = geo.buffer_offset;
+
+	// free any buffers we already own, so that assignment doesn't leak them
+	if (vert != nullptr) {
+		vm_free(vert);
+	}
+	if (radius_list != nullptr) {
+		vm_free(radius_list);
+	}
 
 	if (n_allocated > 0) {
 		vert = (vertex *) vm_malloc( sizeof(vertex) * n_allocated );
@@ -146,6 +157,37 @@ geometry_batcher& geometry_batcher::operator=(const geometry_batcher &geo)
 	return *this;
 }
 
+geometry_batcher::geometry_batcher(geometry_batcher &&other) noexcept
+	: n_to_render(std::exchange(other.n_to_render, 0)),
+	  n_allocated(std::exchange(other.n_allocated, 0)),
+	  vert(std::exchange(other.vert, nullptr)),
+	  use_radius(std::exchange(other.use_radius, true)),
+	  radius_list(std::exchange(other.radius_list, nullptr)),
+	  buffer_offset(std::exchange(other.buffer_offset, -1))
+{
+}
+
+geometry_batcher& geometry_batcher::operator=(geometry_batcher &&other) noexcept
+{
+	if (this != &other) {
+		if (vert != nullptr) {
+			vm_free(vert);
+		}
+		if (radius_list != nullptr) {
+			vm_free(radius_list);
+		}
+
+		n_to_render = std::exchange(other.n_to_render, 0);
+		n_allocated = std::exchange(other.n_allocated, 0);
+		vert = std::exchange(other.vert, nullptr);
+		use_radius = std::exchange(other.use_radius, true);
+		radius_list = std::exchange(other.radius_list, nullptr);
+		buffer_offset = std::exchange(other.buffer_offset, -1);
+	}
+
+	return *this;
+}
+
 /*
 0----1
 |\   |

code/graphics/grbatch.h

@@ -32,8 +32,12 @@ class geometry_batcher
 	geometry_batcher(): n_to_render(0), n_allocated(0), vert(NULL), use_radius(true), radius_list(NULL), buffer_offset(-1) {}
 	~geometry_batcher();
 
-    geometry_batcher(const geometry_batcher &geo) { clone(geo); }
-    geometry_batcher& operator=(const geometry_batcher &geo);
+	// delegate to the default constructor so that clone() sees initialized members
+	geometry_batcher(const geometry_batcher &geo) : geometry_batcher() { clone(geo); }
+	geometry_batcher& operator=(const geometry_batcher &geo);
+
+	geometry_batcher(geometry_batcher &&other) noexcept;
+	geometry_batcher& operator=(geometry_batcher &&other) noexcept;
 
 	// initial memory space needed
 	// NOTE: This MUST be called BEFORE calling any of the draw_*() functions!!

code/mission/missioncampaign.cpp

@@ -1194,6 +1194,35 @@ void mission_campaign_mission_over(bool do_next_mission)
 		mission_campaign_next_mission();			// sets up whatever needs to be set to actually play next mission
 }
 
+void mission_campaign_free_mission_strings(cmission &cm)
+{
+	if (cm.name != nullptr) {
+		vm_free(cm.name);
+		cm.name = nullptr;
+	}
+
+	if (cm.notes != nullptr) {
+		vm_free(cm.notes);
+		cm.notes = nullptr;
+	}
+
+	// the next three are strdup'd return values from parselo.cpp - taylor
+	if (cm.mission_branch_desc != nullptr) {
+		vm_free(cm.mission_branch_desc);
+		cm.mission_branch_desc = nullptr;
+	}
+
+	if (cm.mission_branch_brief_anim != nullptr) {
+		vm_free(cm.mission_branch_brief_anim);
+		cm.mission_branch_brief_anim = nullptr;
+	}
+
+	if (cm.mission_branch_brief_sound != nullptr) {
+		vm_free(cm.mission_branch_brief_sound);
+		cm.mission_branch_brief_sound = nullptr;
+	}
+}
+
 /**
  * Called when the game closes -- to get rid of memory errors for Bounds checker
  * also called at campaign init and campaign load
@@ -1207,36 +1236,12 @@ void mission_campaign_clear()
 	// be sure to remove all old malloced strings of Mission_names
 	// we must also free any goal stuff that was from a previous campaign
 	for ( i=0; i<Campaign.num_missions; i++ ) {
-		if ( Campaign.missions[i].name != NULL ) {
-			vm_free(Campaign.missions[i].name);
-			Campaign.missions[i].name = NULL;
-		}
-
-		if (Campaign.missions[i].notes != NULL) {
-			vm_free(Campaign.missions[i].notes);
-			Campaign.missions[i].notes = NULL;
-		}
+		mission_campaign_free_mission_strings(Campaign.missions[i]);
 
 		Campaign.missions[i].goals.clear();
 		Campaign.missions[i].events.clear();
 		Campaign.missions[i].variables.clear();
 
-		// the next three are strdup'd return values from parselo.cpp - taylor
-		if (Campaign.missions[i].mission_branch_desc != NULL) {
-			vm_free(Campaign.missions[i].mission_branch_desc);
-			Campaign.missions[i].mission_branch_desc = NULL;
-		}
-
-		if (Campaign.missions[i].mission_branch_brief_anim != NULL) {
-			vm_free(Campaign.missions[i].mission_branch_brief_anim);
-			Campaign.missions[i].mission_branch_brief_anim = NULL;
-		}
-
-		if (Campaign.missions[i].mission_branch_brief_sound != NULL) {
-			vm_free(Campaign.missions[i].mission_branch_brief_sound);
-			Campaign.missions[i].mission_branch_brief_sound = NULL;
-		}
-
 		if ( !Fred_running ){
 			sexp_unmark_persistent(Campaign.missions[i].formula);		// free any sexpression nodes used by campaign.
 		}

code/mission/missioncampaign.h

@@ -195,6 +195,10 @@ extern void mission_campaign_mission_over( bool do_next_mission = true );
 // frees all memory at game close time
 extern void mission_campaign_clear( void );
 
+// frees and nulls a mission's five vm_strdup'd strings (name, notes, and the
+// three mission-branch strings)
+extern void mission_campaign_free_mission_strings(cmission &cm);
+
 // used by Fred to get a mission's list of goals.
 void read_mission_goal_list(int num);
 

code/mission/missionparse.cpp

@@ -3366,6 +3366,19 @@ p_object::~p_object()
 	dock_free_dock_list(this);
 }
 
+p_object &p_object::operator=(p_object &&other) noexcept
+{
+	if (this != &other) {
+		// free our dock list, which the memberwise assignment below would otherwise overwrite (and leak)
+		dock_free_dock_list(this);
+
+		// shallow memberwise copy, then transfer ownership of the dock list by nulling the source's handle
+		*this = other;
+		other.dock_list = nullptr;
+	}
+	return *this;
+}
+
 const char* p_object::get_display_name() {
 	if (has_display_name()) {
 		return display_name.c_str();