several fixes to move semantics#7519
Open
Goober5000 wants to merge 5 commits into
Open
Conversation
Goober5000
added
cleanup
Goober5000
force-pushed
the
move_fixes
branch
2 times, most recently
from
ea09071 to
12f2e38
Compare
w_bank: add a move constructor. object_copy_including_array_member returns by value, and without a move constructor the return fell back to the shallow copy constructor whenever NRVO was not applied (e.g. MSVC debug builds), after which the local's destructor deleted the freshly allocated arrays -- leaving the stored bank dangling and double-deleting at model unload. Also add noexcept to the move operations, replace the destructor call in move-assignment with explicit delete[]s (using an object after its lifetime has ended is undefined behavior), guard against self-move, and document that the defaulted copy constructor is intentionally shallow for object_copy_including_array_member's copy-then-replace pattern. LuaThread: declare the defaulted move operations noexcept. This exposed that LuaTable was not movable at all (its user-declared destructor suppressed the implicit moves), so LuaThread's implicit move spec was formally throwing -- a hard error on gcc 9, which still requires the spec on a defaulted redeclaration to match the implicit one. Restore LuaTable's rule of five and declare the LuaValue and LuaFunction copy operations noexcept (they only copy a shared_ptr, a raw pointer, and an enum), making the noexcept claims hold throughout the hierarchy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…message_queue struct safe for moves
clone() allocated fresh vert/radius_list buffers without freeing the ones already owned, so assigning over a live batcher leaked both. It also never copied buffer_offset, and the copy constructor called it with no member initialization at all -- so every copy-constructed batcher carried an uninitialized buffer_offset. Free the existing buffers in clone(), copy buffer_offset, and delegate the copy constructor to the default constructor so clone() sees initialized members (which is also what makes its new frees safe on that path). Also add noexcept move operations, which the user-declared copy operations had suppressed, so by-value handling can transfer the buffers instead of deep-copying them. None of these defects were reachable from current callers (the static batch maps mutate entries in place); this closes the latent hazards. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Five commits, addressing various issues Claude found in an audit of the move semantics in the codebase: