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.
Details
- Reviewers
- None
- Commits
- rP22931: Move CGUIDummyObject class used for empty GUI objects to a separate file, and…
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
source/gui/CGUI.h | ||
---|---|---|
576 ↗ | (On Diff #9426) | While at it, I'd call it m_RootObject as we use objects as a tree. |
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". |
source/gui/CGUI.h | ||
---|---|---|
576 ↗ | (On Diff #9426) | I agree in principle, it would be easier. 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