While reviewing D2810, I noticed that many of the Interfaces queried with queryOwnerInterface don't have safeguards for nonexistance. Usually not a problem since they exists, however they need to be checked.
Details
- Reviewers
Freagarach - Commits
- rP24050: Add safeguards for all queryOwnerInterface calls
grep and see all have safeguards now
Agree with the proposed safeguards
Observe effectively nothing changes
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 13023 Build 25715: Vulcan Build Jenkins Build 25714: Vulcan Build (macOS) Jenkins Build 25713: Vulcan Build (Windows) Jenkins Build 25712: arc lint + arc unit
Time | Test | |
---|---|---|
0 ms | Jenkins > TestComponentScripts::Debug: Test / test_scripts Assertion failed: componentManager->LoadScript(VfsPath(L"simulation/components") / pathname)
Test failed: L"Running script simulation/components/tests/test_ProductionQueue.js"
Assertion failed: scriptInterface.LoadScript(pathname, content) | |
0 ms | Jenkins > TestComponentScripts::Release: Test / test_scripts Assertion failed: componentManager->LoadScript(VfsPath(L"simulation/components") / pathname)
Test failed: L"Running script simulation/components/tests/test_ProductionQueue.js"
Assertion failed: scriptInterface.LoadScript(pathname, content) | |
0 ms | Jenkins > cxxtest-debug-gcc6.xml::[failed-to-read] Failed to read test report file /zpool0/trunk/cxxtest-debug-gcc6.xml
org.dom4j.DocumentException: Error on line 1 of document : Content is not allowed in prolog.
at org.dom4j.io.SAXReader.read(SAXReader.java:511)
| |
0 ms | Jenkins > cxxtest-debug.xml::[failed-to-read] Failed to read test report file /Users/wfg/Jenkins/workspace/macos-differential/cxxtest-debug.xml
org.dom4j.DocumentException: Error on line 1 of document : Content is not allowed in prolog.
at org.dom4j.io.SAXReader.read(SAXReader.java:511)
| |
0 ms | Jenkins > TestAllocators::Debug: Test / test_da | |
View Full Test Results (4 Failed · 684 Passed) |
Event Timeline
Seems good overall
binaries/data/mods/public/simulation/components/ProductionQueue.js | ||
---|---|---|
451 | I feel like this should be a straight warning/error. Seems buggy, particularly since you've already subtracted resources. | |
827 | Likewise -> makes no sense to have a tech template if the owner has no technology manager | |
893 | samesies |
binaries/data/mods/public/simulation/components/Promotion.js | ||
---|---|---|
88 | no player means owner is invalid so entity is mostly dead |
binaries/data/mods/public/simulation/components/ProductionQueue.js | ||
---|---|---|
451 | A second look showed, we already check for cmpTechnologyManager in this.GetTechnologiesList, so strictly we don't need to check this. Don't really like querying the interface twice, but out of scope | |
binaries/data/mods/public/simulation/components/Promotion.js | ||
88 | Afaik the template doesn't contain a player ID | |
binaries/data/mods/public/simulation/helpers/Commands.js | ||
302 | not much to change here, since no cmpPlayerEntityLimits means no limits, hence we just pass by |
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/3086/display/redirect
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1434/display/redirect
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/2533/display/redirect
binaries/data/mods/public/simulation/components/Promotion.js | ||
---|---|---|
88 | I meant using +template.Promotion.RequiredXp. But Angen may be right and it may not be needed. |
This is complete and correct (awaiting the answer).
binaries/data/mods/public/simulation/components/ProductionQueue.js | ||
---|---|---|
383–384 | Should one still be allowed to train a restricted entity when there is no IID_EntityLimits? To me that equals having a limit of 0 so we should return here. |