Page MenuHomeWildfire Games

Delete unused IGUIObject::Destroy and according MEGA TODO
ClosedPublic

Authored by elexis on Sep 20 2019, 3:36 PM.

Details

Summary

rP74 introduced the CGUI::Destroy and IGUIObject::Destroy function and "BIG TODO". In rP141 it was changed to a "MEGA TODO".
Yet the function is empty and we don't have leaks because the destructor works correctly as it should (unless theres some hidden leak, which then would not be related to this method of erasure order).
The destructor is always called in this for-loop, and the destructor must be deleting everything correctly. Whether an IGUIObject wants to call a Destroy helper function, like Minimap.cpp is up to the IGUIObject.

Right now it is not necessary to call IGUIObject destruction recursively, but it iterates through this map, just like it iterates through the map for m_HotkeyObjects to improve performance.
If one does want to change it to recursive erasure, then one still doesnt need a mandatory Destroy function for all GUI object types and can still just call the destructor (even for non-pointers).

The only reason to introduce recursive erasure is when the destructor of one object needs to read from a child object that might have been deleted before already.
But that's not the case, right now all IGUIObject derived classes work for their own in their own compartment.

There are HandleAdditionalChildren functions like COList::HandleAdditionalChildren, they create their own children and take care of them already without those being inserted into that map (see absence of AddToPointersMap and UpdateObjects(), AddChild, AddObject call).
If anything, it could be relevant for CGUI::Xeromyces_ReadTooltip in theory, but in practice it's deleted correctly and the Destroy function is still unused.

I suppose one could argue that this already is a form of recursive deletion, since GUIObjects that consider to "own" child GUI Objects (as in considering them to be the ones in responsibility of deleting them), then those GUIObjects would delete in their destructor, thus recurse.
This could yield double free segfaults in case the map iteration later comes across such deleted child GUIObjects.
That would mean using SAFE_DELETE would be preferable to avoid double free crash in case someone from the future adds a class owning GUIObjects plus adding them to that CGUI map.

Test Plan

Verify that the reasoning in the summary is deductive and every step correct.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Sep 20 2019, 3:36 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/263/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/775/display/redirect

Stan added a subscriber: Stan.Sep 20 2019, 3:47 PM
Stan added inline comments.
source/gui/CGUI.cpp
371 ↗(On Diff #9881)

Why aren't icons cleaned the same way ?

elexis added inline comments.Sep 20 2019, 3:49 PM
source/gui/CGUI.cpp
371 ↗(On Diff #9881)

Why would they be cleaned the same way?

Stan added inline comments.Sep 20 2019, 3:51 PM
source/gui/CGUI.cpp
371 ↗(On Diff #9881)

Cause it's a std:map like the others ?

elexis added inline comments.Sep 20 2019, 4:02 PM
source/gui/CGUI.cpp
371 ↗(On Diff #9881)

Pointers are the ones that are deleted, and only pointers.

Instead we should say m_Icons should be deleted like m_Styles and m_ScrollBarStyles is deleted here, refs rP1085.

elexis updated this revision to Diff 9883.Sep 20 2019, 4:55 PM

Upload something that compiles, delete redundant m_Icons.clear(), delete the other clear calls too.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/265/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/simulation2/helpers/Selection.h
|  34| namespace·EntitySelection
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceEntitySelection{' is invalid C code. Use --std or --language to configure the language.

source/gui/IGUIObject.h
|  41| template·<typename·T>·class·GUI;
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'template<...' is invalid C code. Use --std or --language to configure the language.

source/gui/CGUI.h
|  52| »   std::map<CStr,·CStrW>·m_SettingsDefaults;
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'std::map' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/777/display/redirect

Verified with valgrind for leaks, found only the known ones which sound like leaks in my driver.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 20 2019, 5:25 PM
This revision was automatically updated to reflect the committed changes.