While removing copies in source/gui/, I found a number of places where setting values are copied using GetSetting rather than using the GetSettingPointer. That is all strings and colors are copied, but using a reference for those would be faster.
After writing the patch to switching the copies to GetSettingPointer, I found this pointer code too ugly when this could use references directly, and without the weird custom exceptions that are redundant to std exceptions that noone catches but relies on them being defined by the code (also none of the users check whether the setting type is correct either but rely on that).
Hence this patch introduces a GetSetting reference function that doesn't copy the value nor returns a pointer to avoid both of these issues when just parsing the std::map<CStr, IGUISetting*>.
This way the patches that will remove the GetSetting copies can use GetSetting by reference rather than having to deal with pointers.
Details
- Reviewers
- None
- Commits
- rP22693: Use a new GetSetting returning a reference instead of GetSettingPointer when…
- Trac Tickets
- #5575
Ensure there is no behavior change in this patch, only shorter code and references instead of pointers. Notice that there are no more users of GetSettingPointer except one internally that may be addressed later.
Notice there are a sufficient number of users that mutate the setting (for example every CGUISpriteInstance due to the draw cache), so const ref is not sufficient.
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
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/gui/CInput.h | 23| | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'classCGUIList{' is invalid C code. Use --std or --language to configure the language. source/gui/GUIutil.h | 44| template<typename·T>·class·GUI; | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'template<...' 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/426/display/redirect
Some comments. I'll finish reading after lunch.
source/gui/CButton.cpp | ||
---|---|---|
67 ↗ | (On Diff #9401) | Any reason to not inline this if you are not checking for its value ? |
98 ↗ | (On Diff #9401) | Not const ? |
source/gui/CChart.cpp | ||
192 ↗ | (On Diff #9401) | range loop ? |
source/gui/CCheckBox.cpp | ||
80 ↗ | (On Diff #9401) | Same question about inlining this. |
source/gui/CButton.cpp | ||
---|---|---|
67 ↗ | (On Diff #9401) | line length |
98 ↗ | (On Diff #9401) | CGUISprite has a mutable draw cache |
source/gui/CChart.cpp | ||
192 ↗ | (On Diff #9401) | i is not only used in pSeries.m_Series[i] |
source/gui/CCheckBox.cpp | ||
80 ↗ | (On Diff #9401) | Maybe later, as I still plan to remove at least 5 characters from that line |
source/gui/GUIutil.cpp | ||
---|---|---|
421 ↗ | (On Diff #9401) | If I add a flimsy unused HasSetting to please the ones objecting to missing setting existence checks, I should at least register it for all types. |
Mostly uploading to test against Jenkins, since committing a patch that Jenkins doesnt digest means it wont build revision proposals anymore, so it wouldnt even test the fix to Jenkins (as learnt by experience).
wut a fetish patch
but the tls in a23b is broken DEFAULT since ever half a year+ or wut
but no a23c release? or wut
Test it very good fix crash on Mac http://www.mediafire.com/file/00lxxtdmsu64nrs/0ad-0.0.23b-alpha-osx64.dmg/file