Page MenuHomeWildfire Games

Use GetSetting references instead of GetSettingPointer
ClosedPublic

Authored by elexis on Aug 19 2019, 11:37 AM.
Tags
None
Subscribers
Tokens
"Pterodactyl" token, awarded by elexis."Hungry Hippo" token, awarded by ffffffff."Cup of Joe" token, awarded by nani.

Details

Summary

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.

Test Plan

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

elexis created this revision.Aug 19 2019, 11:37 AM

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

Stan added a subscriber: Stan.Aug 19 2019, 12:13 PM

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.

elexis added inline comments.Aug 19 2019, 12:20 PM
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

elexis added inline comments.Aug 19 2019, 12:22 PM
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.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 19 2019, 12:32 PM
This revision was automatically updated to reflect the committed changes.
nani awarded a token.Aug 19 2019, 1:08 PM
Stan added a comment.Aug 19 2019, 1:16 PM

Well I guess I won't finish reading after lunch then ^^.

elexis added a comment.EditedAug 19 2019, 1:30 PM

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).

ffffffff awarded a token.EditedAug 19 2019, 1:46 PM
ffffffff added a subscriber: ffffffff.

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

Stan added a comment.Aug 19 2019, 2:00 PM

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

elexis updated the Trac tickets for this revision.Aug 29 2019, 11:22 AM