Page MenuHomeWildfire Games

Put CGUI BaseObject / CGUIDummyObject on the stack
ClosedPublic

Authored by elexis on Aug 20 2019, 11:45 PM.

Details

Summary

While cleaning the CGUI and IGUIObject destructor, I noticed that this pointer is created in the constructor and deleted in the destructor, so it's no different from putting it on the stack.
Putting it on the stack means one the destructor becomes more simple and prefering to put things on the stack rather than on the heap means one prefers shorter code and more certainty towards deletion, i.e. safer with regards to leaks.

Test Plan

Notice that if someone wanted to call the IGUIObject destructor at a given time in the CGUI destructor, they can still call the destructor explicitly.
Notice that there already are a number of member variables, so the less there is to delete, the easier it will be to maintain.
Notice that the DummyObject constructor doesn't use the this-pointer other than storing it, especially it doesn't use the ScriptInterface.
It does call the IGUIObject constructor which adds some settings, but those should
(A) probably be deleted in the long run for the DummyObject, and
(B) don't access the ScriptInterface (since creating a setting only creates the setting value, which are only primitives and structs of primitives).

Notice that the circumstances may change in the future, which can certainly mean that someone needs a different init order.
But that doesn't convince me that we should chose pointers by default when things can be on the stack by default, otherwise we could add a lot more pointers and delete calls all over which I hope noone favors.

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.Aug 20 2019, 11:45 PM
vladislavbelov added inline comments.
source/gui/CGUI.h
576 ↗(On Diff #9426)

While at it, I'd call it m_RootObject as we use objects as a tree.

elexis added inline comments.Aug 20 2019, 11:55 PM
source/gui/CGUI.h
576 ↗(On Diff #9426)

Have a look at IsRootObject

source/gui/CGUI.h
576 ↗(On Diff #9426)

Yeah, I saw that. It just seems for me not really needed to have 2 objects in terms of "root".

elexis added inline comments.Aug 21 2019, 12:13 AM
source/gui/CGUI.h
576 ↗(On Diff #9426)

I agree in principle, it would be easier.
But they have different connotation.

If one wants this to be called Root, one will need a different name for the direct children of root, which are the first objects that aren't dummy.

I had removed the IsRootObject function at first, but it seems reusable and more readable than the parent check, so I didn't touch it for a lack of better branding.

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

Linter detected issues:
Executing section Source...

source/gui/CGUI.h
|  50| »   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/441/display/redirect

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

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

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

Linter detected issues:
Executing section Source...

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

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

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
| 211| 
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

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

This revision was not accepted when it landed; it landed in state Needs Review.Sep 18 2019, 10:51 PM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Sep 18 2019, 10:51 PM