As reported by leper in rP19027, the points passed from JS to the C++ are constructed as C++ objects and then copied again by a copy-assignment. The copy may be avoided.
This patch also transforms the ENSURE into a JS_ReportError, which is what the rest of the code does.
Details
- Reviewers
wraitii
Make sure that the move assignment can actually be performed here, that the source vector is discarded regardless.
Observe that ENSURE is actually doing the wrong thing in case a developer clicks on continue (continuing the next statement when the right next step would be to return from this function instead of continuing it.)
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 8564 Build 14019: Vulcan Build Jenkins Build 14018: arc lint + arc unit
Event Timeline
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/simulation2/scripting/EngineScriptConversions.cpp | 188| » » FAIL_VOID("Failed·to·get·Vector3D·constructor"); | | [MAJOR] CPPCheckBear (unknownMacro): | | There is an unknown macro here somewhere. Configuration is required. If STMT is a macro then please configure it. source/scriptinterface/ScriptConversions.cpp | 1| /*·Copyright·(C)·2018·Wildfire·Games. | | [NORMAL] LicenseYearBear: | | License should have "2019" year instead of "2018" Executing section JS... Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/212/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/simulation2/scripting/EngineScriptConversions.cpp | 188| » » FAIL_VOID("Failed·to·get·Vector3D·constructor"); | | [MAJOR] CPPCheckBear (unknownMacro): | | There is an unknown macro here somewhere. Configuration is required. If STMT is a macro then please configure it. source/scriptinterface/ScriptConversions.cpp | 1| /*·Copyright·(C)·2018·Wildfire·Games. | | [NORMAL] LicenseYearBear: | | License should have "2019" year instead of "2018" Executing section JS... Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/213/display/redirect
source/gui/CChart.cpp | ||
---|---|---|
200 | When doing this, the swap itself is O(1) but we call the destructor of all items previously in data.m_Points. However, I think CVector2D has a trivial destructor as it's 2 floats and not user provided, so this does nothing, and thus the whole operation is O(1) as expected. One could also use data.m_Points.swap(pSeries->m_Series[i]), which guarantees constant complexity (we'll still run the destructors later, but that's again not relevant). Now, the question is: is pSeries still used after? Would JS code try to read from it instead of just write to it? Would we call GetSettingPoint(this, "series") elsewhere? |
Checked for usage of the series attribute - it's not used anywhere else right now, so this would be safe to commit. Obviously, it wouldn't be correct if we ever needed to read series from the JS code though.
I'm not sure it's a good thing to commit this. It might break code weirdly, it's a very specific change, and further - is it actually a bottleneck anywhere to copy the vector this way?
source/gui/CChart.cpp | ||
---|---|---|
200 | This change add a strong restriction, you should call UpdateSeries only after the data was changed by a script. I suppose it may be broken. @elexis could you test a case when you updated data once and then update axis_width/size/style multiple times. | |
source/scriptinterface/ScriptConversions.cpp | ||
349 | That's useless and looks strange. | |
source/scriptinterface/ScriptConversions.h | ||
39 | u32 > size_t. |
source/gui/CChart.cpp | ||
---|---|---|
200 | The specs say (
) But actually, why does m_Series exist to begin with - it sounds like it should be stored in the SettingPointer already. One can use series = { values: [], colors: [] } and then parse the color in FromJSVal without copying, even without creating the temporary, without having the member variable at all, unless I miss something. | |
source/scriptinterface/ScriptConversions.h | ||
39 | (JS_SetElement expects unsigned int, but one may prefer size_t since size() is in it and since the val[i] uses that, rather than the destination JS_SetElement, whatever) | |
44 | The index. |