Fix three memory leaks and make the debug leak check accurate - #7781
Conversation
ce998ff to
40153d0
Compare
Goober5000
changed the title
| Num_path_restrictions = 0; | ||
| Num_ai_dock_names = 0; | ||
| ai_clear_goal_target_names(); | ||
| Goal_target_names.clear(); |
There was a problem hiding this comment.
This could really use a shrink_to_fit() as well to release the memory allocated by the vector. Not a huge deal here since it's just a vector or pointers, but it's good to actually clean things up properly, or at least add a comment indicating why it's not done (in cases where there is a reason, and here could arguably be one such case).
Actually there look to be quite a few places where vectors are cleared but their memory isn't released. And we could/should really move some of this stuff into a mission_close() so that things things are cleaned up once a mission ends instead of hanging around until a new mission begins. In particular I'm thinking about standalone servers, where they often hang around for long periods between between missions and are generally more memory constrained. All of that is obviously out of scope for this PR but might be something to look into with any followups.
There was a problem hiding this comment.
Standard practice is to not use shrink_to_fit() unless it's really needed. In this case, Goal_target_names is usually repopulated right away, so freeing and reallocating the memory would just be thrashing with no benefit. So, clear() by itself is to be expected, most of the time.
Fair points about mission_close() and standalones, but yes out of scope for this PR.
| for (auto ptr : Goal_target_names) | ||
| vm_free(ptr); | ||
| Goal_target_names.clear(); | ||
| Goal_target_names.emplace_back(vm_strdup(name)); |
There was a problem hiding this comment.
Pretty sure push_back() should be preferred here as it's my understanding that emplace_back() is doing far more work in comparison given the usage.
| vm_free(ptr); | ||
| Goal_target_names.clear(); | ||
| Goal_target_names.emplace_back(vm_strdup(name)); | ||
| return Goal_target_names[*index].get(); |
There was a problem hiding this comment.
I would think just using back().get() here is more direct and clear visually. Is there a good reason not to do that?
There was a problem hiding this comment.
It was how the code was before, but yes that's a good improvement.
|
Hmm, now the content of 7782 isn't showing here. Maybe github is having issues? Might be good to check it on your side just to be sure. |
Report leaks after static destruction via _CRTDBG_LEAK_CHECK_DF instead of calling _CrtDumpMemoryLeaks() at the end of main(), which incorrectly reported every global container as a memory leak. Also fix the three remaining real leaks which were discovered after all the noise was cleared up: - Mc_point_list was never freed, because its atexit cleanup was only registered by code that no longer runs. But the list was only a temporary array of pointers used while building the collision tree, so remove it and have model_collide_parse_bsp_defpoints() copy the vertices directly into the tree's point list. - model_batch_buffer's Mem_alloc copy was never freed. But the copy is actually not necessary, so remove it and pass Submodel_matrices directly. - Goal_target_names strings were only freed when the next mission was parsed, so the last mission's names leaked at exit. Hold them in SCP_vm_unique_ptr so they are freed with the vector. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
40153d0 to
519e3ae
Compare
2 participants
Report leaks after static destruction via _CRTDBG_LEAK_CHECK_DF instead of calling _CrtDumpMemoryLeaks() at the end of main(), which incorrectly reported every global container as a memory leak.
Also fix the three remaining real leaks which were discovered after all the noise was cleared up: