Page MenuHomeWildfire Games

Put CGUI BaseObject / CGUIDummyObject on the stack
Needs ReviewPublic

Authored by elexis on Tue, Aug 20, 11:45 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
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
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8982
Build 14718: Vulcan BuildJenkins
Build 14717: arc lint + arc unit

Event Timeline

elexis created this revision.Tue, Aug 20, 11:45 PM
vladislavbelov added inline comments.
source/gui/CGUI.h
576

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

elexis added inline comments.Tue, Aug 20, 11:55 PM
source/gui/CGUI.h
576

Have a look at IsRootObject

source/gui/CGUI.h
576

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

elexis added inline comments.Wed, Aug 21, 12:13 AM
source/gui/CGUI.h
576

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