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.
Details
- Reviewers
- None
- Trac Tickets
- #4199
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 Build Jenkins Build 14769: arc lint + arc unit
Event Timeline
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
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. |
source/simulation2/Simulation2.cpp | ||
---|---|---|
360 | Makes sense :) |