Page MenuHomeWildfire Games

Delete CSimulation2 m_MapSettings and redundant getters
Needs ReviewPublic

Authored by elexis on Aug 22 2019, 11:48 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Trac Tickets
#4199
Summary

D2197 exposes m_InitAttributes of CSimulation2 to the GUIInterface. As a protective measure, ScriptInterface::FreezeObject is called on that variable to avoid unintentional modifications to the data (see #4257 for examples of where non-frozen objects were unintentionally modfied resulting in tedious camouflaged bugs).
Inserting the Freeze on SetInitAttributes had shown that m_InitAttributes is modified by the LoadMapSettings call which only operates on m_MapSettings, indicating that m_MapSettings is not an independent JS value but a reference to the object that is the "settings" property of m_InitAttributes.
Therefore it is misleading to the reader to use two member variables.
The alternative is having one variable and a getter function that returns the setting.
This at least means everyone is aware that there is only one variable where such settings can be stored, thus that only one such instance exists.
The unexpectation is not completely resolved, because the getter also returns something that people might confuse with a copy.
Hence add the comment warning everyone about that.

Test Plan

Am I just imagining this or am I right? Prove me right or wrong. The Freeze call is one way to demonstrate this being a reference. But one should check every line.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 9016
Build 14770: Vulcan BuildJenkins
Build 14769: arc lint + arc unit

Event Timeline

elexis created this revision.Aug 22 2019, 11:48 PM

Build failure - The Moirai have given mortals hearts that can endure.

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

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

Linter detected issues:

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

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

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

Stan added a subscriber: Stan.Aug 28 2019, 12:45 PM
Stan added inline comments.
source/simulation2/Simulation2.cpp
194

Isn't that already implied by the comment you put in the header file ?

360

a && b instead of !a || !b ?

372

a && b instead of !a || !b ?

Krinkle added inline comments.Sep 1 2019, 1:43 AM
source/simulation2/Simulation2.cpp
360

!a || !b !== ( a && b ).

!a || !b === !( a && b ).

If there was an if and an else, I think a && b would be more readable, and then you'd swap the if/else body. But in this case, there is no else. The line after it is unconditional.

Stan added inline comments.Sep 1 2019, 10:51 AM
source/simulation2/Simulation2.cpp
360

Makes sense :)

elexis updated the Trac tickets for this revision.Oct 13 2019, 4:53 AM
Stan added a subscriber: bb.Mar 6 2022, 3:43 PM

@bb Anything of interest to you here?