Files
Goober5000andClaude Fable 5 f90a0b3c70 fix four more shallow-ownership hazards in by-value types
A sweep for the w_bank pattern (raw owning handles in types that are
copied, moved, or shifted by value) found four more instances:

p_object: the destructor frees dock_list, which suppresses the implicit
move operations, so Parse_objects growth copied elements -- safe only
because dock lists happen to be built after parsing finishes.  Wrap
dock_list in the new util::reset_on_move<> (a handle whose move resets
the source) and explicitly default the rule of five (with a user-defined
move assignment that frees any existing dock list before the memberwise
transfer), so moves now transfer the dock list.

DecalDefinition: the defaulted moves copied the bitmap handles without
resetting the source, so destroying a moved-from element would
bm_release live handles -- safe only because DecalDefinitions grows
before loadBitmaps runs.  Wrap the handles in reset_on_move<int, -1>.

cmission: FRED's remove_mission overwrote the removed slot's five
vm_strdup'd strings without freeing them (a leak on every removal) and
left the vacated slot aliasing the surviving copy (a double-free if the
slot is later reused without reassigning every string).  Factor the
string cleanup out of mission_campaign_clear into
mission_campaign_free_mission_strings and call it before the overwrite,
nulling the vacated slot afterward.

Undo_stack: the destructor deletes the tracked items, but the implicit
shallow copy was used to transfer stacks into the undo system, kept safe
only by an immediately-following untrack() call.  Delete the copy
operations, default the move constructor and implement move assignment
as clear-then-exchange, so an assigned-over stack deletes its own items
first and the source is guaranteed empty.

Co-Authored-By: Claude Fable 5 <[email protected]>
2026-07-06 13:33:49 -04:00

175 lines
3.6 KiB
C++

/*
* z64555's Undo system, created for the FreeSpace Source Code project
*/
#include "globalincs/pstypes.h"
#include "globalincs/undosys.h"
#include "globalincs/vmallocator.h"
#include <utility>
// Pure virtual deconstructors must be defined.
Undo_item_base::~Undo_item_base() = default;
Undo_system::Undo_system()
: max_undos(10) {};
Undo_system::Undo_system(size_t _undos)
: max_undos(_undos) {};
void Undo_system::clamp_stacks() {
while (undo_stack.size() > max_undos) {
// If we're past the stack limit, de-construct and remove until we're in the limit
// If max_undos is held constant (as it should be), the loop isn't necassary.
// But, it also isn't that much different from an if{} when assembled, so there's no harm in using it
delete undo_stack.front();
undo_stack.pop_front();
}
// This while() shouldn't ever be triggered, but this is a safety to prevent any weird edge-cases.
while (redo_stack.size() > max_undos) {
delete redo_stack.front();
redo_stack.pop_front();
}
}
void Undo_system::clear() {
clear_redo();
for (auto &element : undo_stack) {
delete element;
}
undo_stack.clear();
}
void Undo_system::clear_redo() {
for (auto &element : redo_stack) {
delete element;
}
redo_stack.clear();
}
size_t Undo_system::save_stack(Undo_stack&& stack) {
if (stack.size() == 0) {
return 0;
}
clear_redo();
// 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);
clamp_stacks();
return undo_stack.size();
}
std::pair<const void*, const void*> Undo_system::redo() {
std::pair<const void*, const void*> retval;
if (redo_stack.empty()) {
// Nothing to redo
return {nullptr, nullptr};
}
auto &item = redo_stack.back();
retval = item->restore();
undo_stack.push_back(item);
redo_stack.pop_back();
return retval;
};
std::pair<const void*, const void*> Undo_system::undo() {
std::pair<const void*, const void*> retval;
if (undo_stack.empty()) {
// Nothing to undo
return {nullptr, nullptr};
}
auto &item = undo_stack.back();
retval = item->restore();
redo_stack.push_back(item);
undo_stack.pop_back();
return retval;
};
bool Undo_system::empty() {
return undo_stack.empty();
}
bool Undo_system::empty_redo() {
return redo_stack.empty();
}
size_t Undo_system::size() {
return undo_stack.size();
}
size_t Undo_system::size_redo() {
return redo_stack.size();
}
Undo_stack::Undo_stack()
: reverse(false) {
}
Undo_stack::Undo_stack(size_t size)
: reverse(false) {
stack.reserve(size);
}
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);
}
std::pair<const void*, const void*> Undo_stack::restore() {
std::pair<const void*, const void*> retval;
if (reverse) {
for (auto it = stack.rbegin(); it != stack.rend(); ++it) {
retval = (*it)->restore();
}
} else {
for (auto &element : stack) {
retval = element->restore();
}
}
reverse = !reverse;
return retval;
}
size_t Undo_stack::size() {
return stack.size();
}
void Undo_stack::clear() {
for (auto &element : stack) {
delete element;
}
stack.clear();
}