make ship move-correct

Ship's implicit move operations compiled but were semantically broken in
two ways: moving copied the subsys_list sentinel's links verbatim,
leaving the bookend nodes of the intrusive subsystem list pointing at
the old sentinel address; and the raw owning warpin_effect /
warpout_effect pointers were copied without nulling the source, so a
vacated Ships[] slot aliased live WarpEffects that ship::clear() (on
slot reuse) or ship_close() (which walks every slot) would delete out
from under the destination ship.

Fix both at the member level so the compiler-generated moves are
correct by composition:

- Add ship_subsys_sentinel, derived from ship_subsys, whose move
  operations re-link the bookend nodes (handling the null-linked,
  self-referential-empty, and populated source states) and leave the
  moved-from sentinel as an initialized empty list.  Copying is
  deleted.  Public inheritance keeps &ship::subsys_list convertible to
  ship_subsys*, so list_init, GET_FIRST, and every other call site is
  unchanged.

- Convert warpin_effect and warpout_effect to std::unique_ptr.  The
  delete-and-null blocks in ship::clear(), ship_close(), and
  ship_set_warp_effects() collapse into reset(); call sites that borrow
  the pointer into a raw local now use get().

With the members handling themselves, declare ship's special member
functions: defaulted default constructor and moves, deleted copies
(previously implicit via the unique_ptr members, now stated intent).
The moves are deliberately not declared noexcept because members like
SCP_map and std::deque do not have standard-guaranteed nothrow moves;
the computed exception specification is used instead.

Co-Authored-By: Claude Fable 5 <[email protected]>

code/asteroid/asteroid.cpp

@@ -1439,9 +1439,9 @@ int asteroid_check_collision(object *pasteroid, object *other_obj, vec3d *hitpos
 		// everything above was calculated relative to the heavy's position
 		vec3d actual_world_hit_pos = mc.hit_point_world + heavy_obj->pos;
 		if ((shipp->is_arriving()) && (shipp->warpin_effect != nullptr))
-			warp_effect = shipp->warpin_effect;
+			warp_effect = shipp->warpin_effect.get();
 		else if ((shipp->flags[Ship::Ship_Flags::Depart_warp]) && (shipp->warpout_effect != nullptr))
-			warp_effect = shipp->warpout_effect;
+			warp_effect = shipp->warpout_effect.get();
 
 		if (warp_effect != nullptr && point_is_clipped_by_warp(&actual_world_hit_pos, warp_effect))
 			mc_ret_val = 0;

code/debris/debris.cpp

@@ -1152,9 +1152,9 @@ int debris_check_collision(object *pdebris, object *other_obj, vec3d *hitpos, co
 		// everything above was calculated relative to the heavy's position
 		vec3d actual_world_hit_pos = mc.hit_point_world + heavy_obj->pos;
 		if ((shipp->is_arriving()) && (shipp->warpin_effect != nullptr))
-			warp_effect = shipp->warpin_effect;
+			warp_effect = shipp->warpin_effect.get();
 		else if ((shipp->flags[Ship::Ship_Flags::Depart_warp]) && (shipp->warpout_effect != nullptr))
-			warp_effect = shipp->warpout_effect;
+			warp_effect = shipp->warpout_effect.get();
 
 		if (warp_effect != nullptr && point_is_clipped_by_warp(&actual_world_hit_pos, warp_effect))
 			mc_ret_val = 0;

code/graphics/shadows.cpp

@@ -537,8 +537,8 @@ static bool shadow_obj_clip_plane(const object* objp, shadow_render_list::clip_p
 
 	if (dfi.maintained_variables.bool_value) {
 		auto* dship = &Ships[dfi.maintained_variables.objp_value->instance];
-		WarpEffect* warp_effect = dship->is_arriving(ship::warpstage::BOTH, true)
-			? dship->warpin_effect : dship->warpout_effect;
+		auto* warp_effect = (dship->is_arriving(ship::warpstage::BOTH, true)
+			? dship->warpin_effect : dship->warpout_effect).get();
 		model_render_params dummy;
 		if (warp_effect->warpShipClip(&dummy)) {
 			clip->normal = dummy.get_clip_plane_normal();

code/model/modelrender.cpp

@@ -1662,11 +1662,11 @@ void model_render_glowpoint_bitmap(int point_num, const vec3d *pos, const matrix
 
 		if ((shipp->is_arriving()) && (shipp->warpin_effect != nullptr)
 			&& Warp_params[shipp->warpin_params_index].warp_type != WT_HYPERSPACE) {
-			warp_effect = shipp->warpin_effect;
+			warp_effect = shipp->warpin_effect.get();
 		}
 		else if ((shipp->flags[Ship::Ship_Flags::Depart_warp]) && (shipp->warpout_effect != nullptr) 
 			&& Warp_params[shipp->warpout_params_index].warp_type != WT_HYPERSPACE) {
-			warp_effect = shipp->warpout_effect;
+			warp_effect = shipp->warpout_effect.get();
 		}
 
 		if (warp_effect != nullptr && point_is_clipped_by_warp(&world_pnt, warp_effect))
@@ -1821,10 +1821,10 @@ void model_render_glowpoint_add_light(int point_num, const vec3d *pos, const mat
 		WarpEffect* warp_effect = nullptr;
 
 		if ((shipp->is_arriving()) && Warp_params[shipp->warpin_params_index].warp_type != WT_HYPERSPACE) {
-			warp_effect = shipp->warpin_effect;
+			warp_effect = shipp->warpin_effect.get();
 		}
 		else if ((shipp->flags[Ship::Ship_Flags::Depart_warp]) && Warp_params[shipp->warpout_params_index].warp_type != WT_HYPERSPACE) {
-			warp_effect = shipp->warpout_effect;
+			warp_effect = shipp->warpout_effect.get();
 		}
 
 		if (warp_effect != nullptr && point_is_clipped_by_warp(&world_pnt, warp_effect))
@@ -2234,11 +2234,11 @@ void model_queue_render_thrusters(const model_render_params *interp, const polym
 
 				if ((shipp->is_arriving()) && (shipp->warpin_effect != nullptr)
 					&& Warp_params[shipp->warpin_params_index].warp_type != WT_HYPERSPACE) {
-					warp_effect = shipp->warpin_effect;
+					warp_effect = shipp->warpin_effect.get();
 				}
 				else if ((shipp->flags[Ship::Ship_Flags::Depart_warp]) && (shipp->warpout_effect != nullptr)
 					&& Warp_params[shipp->warpout_params_index].warp_type != WT_HYPERSPACE) {
-					warp_effect = shipp->warpout_effect;
+					warp_effect = shipp->warpout_effect.get();
 				}
 
 				if (warp_effect != nullptr && point_is_clipped_by_warp(&world_pnt, warp_effect))

code/object/collideshipship.cpp

@@ -358,9 +358,9 @@ int ship_ship_check_collision(collision_info_struct *ship_ship_hit_info)
 		vec3d actual_world_hit_pos = mc.hit_point_world + heavy_obj->pos;
 
 		if (heavy_shipp->flags[Ship::Ship_Flags::Depart_warp] && heavy_shipp->warpout_effect != nullptr)
-			warp_effect = heavy_shipp->warpout_effect;
+			warp_effect = heavy_shipp->warpout_effect.get();
 		else if (heavy_shipp->flags[Ship::Ship_Flags::Arriving_stage_2] && heavy_shipp->warpin_effect != nullptr)
-			warp_effect = heavy_shipp->warpin_effect;
+			warp_effect = heavy_shipp->warpin_effect.get();
 
 		bool heavy_warp_no_collide = false;
 		if (warp_effect != nullptr)
@@ -368,9 +368,9 @@ int ship_ship_check_collision(collision_info_struct *ship_ship_hit_info)
 
 		warp_effect = nullptr;
 		if (light_shipp->flags[Ship::Ship_Flags::Depart_warp] && light_shipp->warpout_effect != nullptr)
-			warp_effect = light_shipp->warpout_effect;
+			warp_effect = light_shipp->warpout_effect.get();
 		else if (light_shipp->flags[Ship::Ship_Flags::Arriving_stage_2] && light_shipp->warpin_effect != nullptr)
-			warp_effect = light_shipp->warpin_effect;
+			warp_effect = light_shipp->warpin_effect.get();
 
 		bool light_warp_no_collide = false;
 		if (warp_effect != nullptr)

code/object/collideshipweapon.cpp

@@ -435,9 +435,9 @@ static std::tuple<bool, bool, ship_weapon_collision_data> ship_weapon_check_coll
 		WarpEffect* warp_effect = nullptr;
 
 		if (shipp->flags[Ship::Ship_Flags::Depart_warp] && shipp->warpout_effect != nullptr) 
-			warp_effect = shipp->warpout_effect;
+			warp_effect = shipp->warpout_effect.get();
 		else if (shipp->flags[Ship::Ship_Flags::Arriving_stage_2] && shipp->warpin_effect != nullptr)
-			warp_effect = shipp->warpin_effect;
+			warp_effect = shipp->warpin_effect.get();
 
 		bool hull_no_collide, shield_no_collide;
 		hull_no_collide = shield_no_collide = false;

code/prop/prop.cpp

@@ -1094,9 +1094,9 @@ int prop_check_collision(object* prop_obj, object* other_obj, vec3d* hitpos, col
 		// everything above was calculated relative to the heavy's position
 		vec3d actual_world_hit_pos = mc.hit_point_world + heavy_obj->pos;
 		if ((shipp->is_arriving()) && (shipp->warpin_effect != nullptr))
-			warp_effect = shipp->warpin_effect;
+			warp_effect = shipp->warpin_effect.get();
 		else if ((shipp->flags[Ship::Ship_Flags::Depart_warp]) && (shipp->warpout_effect != nullptr))
-			warp_effect = shipp->warpout_effect;
+			warp_effect = shipp->warpout_effect.get();
 
 		if (warp_effect != nullptr && point_is_clipped_by_warp(&actual_world_hit_pos, warp_effect))
 			mc_ret_val = 0;

code/ship/ship.cpp

@@ -6790,6 +6790,13 @@ vec3d get_submodel_offset(int model, int submodel){
 
 }
 
+// Defaulted here rather than in ship.h because the unique_ptr<WarpEffect>
+// members need the complete WarpEffect type at the point of definition.
+ship::ship() = default;
+ship::~ship() = default;
+ship::ship(ship &&) = default;				// NOLINT(performance-noexcept-move-constructor)
+ship &ship::operator=(ship &&) = default;	// NOLINT(performance-noexcept-move-constructor)
+
 // Reset all ship values to empty/unused.
 void ship::clear()
 {
@@ -6824,12 +6831,8 @@ void ship::clear()
 	really_final_death_time = timestamp(-1);
 	deathroll_rotvel = vmd_zero_vector;
 
-	if (warpin_effect != nullptr)
-		delete warpin_effect;
-	if (warpout_effect != nullptr)
-		delete warpout_effect;
-	warpin_effect = nullptr;
-	warpout_effect = nullptr;
+	warpin_effect.reset();
+	warpout_effect.reset();
 
 	warpin_params_index = -1;
 	warpout_params_index = -1;
@@ -16553,13 +16556,8 @@ void ship_close()
 	for (i=0; i<MAX_SHIPS; i++ )	{
 		ship *shipp = &Ships[i];
 
-		if(shipp->warpin_effect != NULL)
-			delete shipp->warpin_effect;
-		shipp->warpin_effect = NULL;
-
-		if(shipp->warpout_effect != NULL)
-			delete shipp->warpout_effect;
-		shipp->warpout_effect = NULL;
+		shipp->warpin_effect.reset();
+		shipp->warpout_effect.reset();
 	}
 
 	// free this too! -- Goober5000

code/ship/ship.h

@@ -450,6 +450,64 @@ public:
 	void clear();
 };
 
+// The head sentinel of a ship's intrusive subsystem list.  The sentinel's
+// address is meaningful: the first node's prev and the last node's next point
+// back at it, and an empty list is self-referential (after list_init) or
+// null-linked (after construction or ship::clear).  Moving a ship would
+// otherwise leave the bookend nodes pointing at the old sentinel address, so
+// the move operations re-link them.  Copying is deleted: a copied sentinel
+// would alias another list's nodes.
+struct ship_subsys_sentinel : public ship_subsys
+{
+	ship_subsys_sentinel() = default;
+
+	ship_subsys_sentinel(const ship_subsys_sentinel &) = delete;
+	ship_subsys_sentinel &operator=(const ship_subsys_sentinel &) = delete;
+
+	ship_subsys_sentinel(ship_subsys_sentinel &&other) noexcept
+	{
+		take_links_from(other);
+	}
+
+	ship_subsys_sentinel &operator=(ship_subsys_sentinel &&other) noexcept
+	{
+		if (this != &other)
+		{
+			Assertion(next == nullptr || next == this, "Move-assigning over a sentinel whose subsystem list is not empty!  The destination's subsystems would be orphaned.");
+			take_links_from(other);
+		}
+		return *this;
+	}
+
+private:
+	void take_links_from(ship_subsys_sentinel &other) noexcept
+	{
+		if (other.next == nullptr)
+		{
+			// other was never list_init'd; match that state
+			next = nullptr;
+			prev = nullptr;
+		}
+		else if (other.next == &other)
+		{
+			// other is an initialized empty list
+			next = this;
+			prev = this;
+		}
+		else
+		{
+			// take ownership of the chain and re-point the bookends
+			next = other.next;
+			prev = other.prev;
+			next->prev = this;
+			prev->next = this;
+		}
+		// leave other as an initialized empty list, which is safe to iterate
+		other.next = &other;
+		other.prev = &other;
+	}
+};
+
 // structure for subsystems which tells us the total count of a particular type of subsystem (i.e.
 // we might have 3 engines), and the relative strength of the subsystem.  The #defines in model.h
 // for SUBSYSTEM_xxx will be used as indices into this array.
@@ -592,8 +650,8 @@ public:
 	int	really_final_death_time;	// Time until ship breaks up and disappears
 	vec3d	deathroll_rotvel;			// Desired death rotational velocity
 
-	WarpEffect *warpin_effect;
-	WarpEffect *warpout_effect;
+	std::unique_ptr<WarpEffect> warpin_effect;
+	std::unique_ptr<WarpEffect> warpout_effect;
 
 	int warpin_params_index;
 	int warpout_params_index;
@@ -655,7 +713,7 @@ public:
 	// of a particular subsystem, like engines).  The subsys_info struct is information for particular
 	// types of subsystems.  (i.e. the list might contain 3 engines.  There will be one subsys_info entry
 	// describing the state of all engines combined) -- MWA 4/1/97
-	ship_subsys	subsys_list;									//	linked list of subsystems for this ship.
+	ship_subsys_sentinel	subsys_list;						//	linked list of subsystems for this ship.
 	std::unique_ptr<ship_subsys*[]> subsys_list_indexer;		//	provides random-access lookup to the linked list
 	ship_subsys	*last_targeted_subobject[MAX_PLAYERS];	// Last subobject that has been targeted.  NULL if none;(player specific)
 	ship_subsys_info	subsys_info[SUBSYSTEM_MAX];		// info on particular generic types of subsystems	
@@ -851,6 +909,28 @@ public:
 		BOTH,
 	};
 
+	// Ships support moving but not copying.  The defaulted moves are correct
+	// because the members carry the smarts: the subsys_list sentinel re-links
+	// its bookend nodes, and the owning pointers are unique_ptrs.  Callers must
+	// still fix up external back-references (Objects[].instance,
+	// Ai_info[].shipnum, etc.) -- see reassign_ship_slot in
+	// missioneditor/common.cpp.
+	// All the defaulted special member functions are defaulted in ship.cpp
+	// rather than here because they need the complete WarpEffect type.
+	// The moves are not declared noexcept because the implicit exception
+	// specification differs by standard library: MSVC and clang compute
+	// nothrow, but libstdc++ computes potentially-throwing for some members,
+	// making an explicit noexcept ill-formed there (a defaulted redeclaration
+	// may not strengthen the implicit specification).  The NOLINTs suppress
+	// clang-tidy's performance-noexcept-move-constructor, which only sees
+	// clang's computation.
+	ship();
+	~ship();
+	ship(const ship &) = delete;
+	ship &operator=(const ship &) = delete;
+	ship(ship &&);				// NOLINT(performance-noexcept-move-constructor)
+	ship &operator=(ship &&);	// NOLINT(performance-noexcept-move-constructor)
+
 	// reset to a completely blank ship
 	void clear();
 

code/ship/shipfx.cpp

@@ -1264,9 +1264,9 @@ void shipfx_emit_spark( int n, int sn )
 	WarpEffect* warp_effect = nullptr;
 
 	if ((shipp->is_arriving()) && (shipp->warpin_effect))
-		warp_effect = shipp->warpin_effect;
+		warp_effect = shipp->warpin_effect.get();
 	else if ((shipp->flags[Ship::Ship_Flags::Depart_warp]) && (shipp->warpout_effect))
-		warp_effect = shipp->warpout_effect;
+		warp_effect = shipp->warpout_effect.get();
 
 	if (warp_effect != nullptr && point_is_clipped_by_warp(&outpnt, warp_effect))
 		return;
@@ -2950,50 +2950,44 @@ void ship_set_warp_effects(object *objp)
 	if (warpout_type & WT_DEFAULT_WITH_FIREBALL)
 		warpout_type = WT_DEFAULT;
 
-	if (shipp->warpin_effect != nullptr)
-		delete shipp->warpin_effect;
-
 	switch (warpin_type)
 	{
 		case WT_DEFAULT:
 		case WT_KNOSSOS:
 		case WT_DEFAULT_THEN_KNOSSOS:
-			shipp->warpin_effect = new WE_Default(objnum, WarpDirection::WARP_IN);
+			shipp->warpin_effect = std::make_unique<WE_Default>(objnum, WarpDirection::WARP_IN);
 			break;
 		case WT_IN_PLACE_ANIM:
-			shipp->warpin_effect = new WE_BSG(objnum, WarpDirection::WARP_IN);
+			shipp->warpin_effect = std::make_unique<WE_BSG>(objnum, WarpDirection::WARP_IN);
 			break;
 		case WT_SWEEPER:
-			shipp->warpin_effect = new WE_Homeworld(objnum, WarpDirection::WARP_IN);
+			shipp->warpin_effect = std::make_unique<WE_Homeworld>(objnum, WarpDirection::WARP_IN);
 			break;
 		case WT_HYPERSPACE:
-			shipp->warpin_effect = new WE_Hyperspace(objnum, WarpDirection::WARP_IN);
+			shipp->warpin_effect = std::make_unique<WE_Hyperspace>(objnum, WarpDirection::WARP_IN);
 			break;
 		default:
-			shipp->warpin_effect = new WarpEffect();
+			shipp->warpin_effect = std::make_unique<WarpEffect>();
 	}
 
-	if (shipp->warpout_effect != nullptr)
-		delete shipp->warpout_effect;
-
 	switch (warpout_type)
 	{
 		case WT_DEFAULT:
 		case WT_KNOSSOS:
 		case WT_DEFAULT_THEN_KNOSSOS:
-			shipp->warpout_effect = new WE_Default(objnum, WarpDirection::WARP_OUT);
+			shipp->warpout_effect = std::make_unique<WE_Default>(objnum, WarpDirection::WARP_OUT);
 			break;
 		case WT_IN_PLACE_ANIM:
-			shipp->warpout_effect = new WE_BSG(objnum, WarpDirection::WARP_OUT);
+			shipp->warpout_effect = std::make_unique<WE_BSG>(objnum, WarpDirection::WARP_OUT);
 			break;
 		case WT_SWEEPER:
-			shipp->warpout_effect = new WE_Homeworld(objnum, WarpDirection::WARP_OUT);
+			shipp->warpout_effect = std::make_unique<WE_Homeworld>(objnum, WarpDirection::WARP_OUT);
 			break;
 		case WT_HYPERSPACE:
-			shipp->warpout_effect = new WE_Hyperspace(objnum, WarpDirection::WARP_OUT);
+			shipp->warpout_effect = std::make_unique<WE_Hyperspace>(objnum, WarpDirection::WARP_OUT);
 			break;
 		default:
-			shipp->warpout_effect = new WarpEffect();
+			shipp->warpout_effect = std::make_unique<WarpEffect>();
 	}
 }
 

code/weapon/beam.cpp

@@ -3216,9 +3216,9 @@ int beam_collide_ship(obj_pair *pair)
 		WarpEffect* warp_effect = nullptr;
 
 		if (shipp->flags[Ship::Ship_Flags::Depart_warp] && shipp->warpout_effect != nullptr)
-			warp_effect = shipp->warpout_effect;
+			warp_effect = shipp->warpout_effect.get();
 		else if (shipp->flags[Ship::Ship_Flags::Arriving_stage_2] && shipp->warpin_effect != nullptr)
-			warp_effect = shipp->warpin_effect;
+			warp_effect = shipp->warpin_effect.get();
 
 
 		bool hull_no_collide, shield_no_collide;