Page MenuHomeWildfire Games

Remove unused variable and fail to register component if it has no constructor [ComponentManager]
ClosedPublic

Authored by Angen on Apr 4 2019, 9:25 PM.

Details

Reviewers
wraitii
elexis
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22476: Remove the unused variable proto from the ComponentManager in rP14877.
Summary

Removes unused variable:
Introduced in rP14877. The variable proto is not used in its scope and function called on assignment does nothing with the object itself (what would affect it later) so can be removed.

There is assert check inside that function for isObjectOrNull but it is done in this scope already.

Adds error messages on fail
Fails if component does not have constructor = is null

Test Plan

Try to register component without constructor -> should tells you nicely where is problem

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

Angen created this revision.Apr 4 2019, 9:25 PM
Owners added a subscriber: Restricted Owners Package.Apr 4 2019, 9:25 PM
Vulcan added a subscriber: Vulcan.Apr 4 2019, 9:27 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1169/display/redirect

wraitii added reviewers: wraitii, Restricted Owners Package.Apr 22 2019, 9:35 AM
elexis retitled this revision from Remove not needed variable and conversion to object to Remove unused variable and conversion in the ComponentManager.Apr 25 2019, 12:31 PM
elexis added a subscriber: elexis.Jul 13 2019, 1:56 PM
elexis added inline comments.
source/simulation2/system/ComponentManager.cpp
263 ↗(On Diff #7669)

m_ScriptInterface.GetProperty(ctor, "prototype", is called twice, just above, and the first call assumes that it is an object, it seems sane to fix it if one wants to make a statement below about that check, but I guess one can also just only remove the unused variable.

267 ↗(On Diff #7669)

As mentioned in rP14877, this case is triggered when passing an empty object, so it's good to keep it as a safeguard. One can reproduce it by
replacing Engine.RegisterComponentType(IID_AlertRaiser, "AlertRaiser", AlertRaiser); with `Engine.RegisterComponentType(IID_AlertRaiser, "AlertRaiser", {}); after gameload for instance.

EnumeratePropertyNamesWithPrefix supports Null.
I wonder if it should complain if the simulation component has no constructor?

Angen added inline comments.Jul 14 2019, 8:33 AM
source/simulation2/system/ComponentManager.cpp
267 ↗(On Diff #7669)

In summary I was referring to that there is check for isObjectOrNull inside protoVal.toObjectNull() and saying that we have the check for isObjectOrNull before calling line 271 :)

Angen updated this revision to Diff 8882.Jul 14 2019, 9:36 AM
Angen retitled this revision from Remove unused variable and conversion in the ComponentManager to Improve ComponentManager.
Angen edited the summary of this revision. (Show Details)

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

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

Angen retitled this revision from Improve ComponentManager to Remove unused variable and fail to register component if it has no constructor [ComponentManager].Jul 14 2019, 10:27 AM
Angen edited the test plan for this revision. (Show Details)
Angen updated this revision to Diff 8893.Jul 14 2019, 1:26 PM

inline

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

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

elexis accepted this revision.Jul 14 2019, 4:05 PM

Thanks for taking the review remarks here and on IRC into account, and extending the scope slightly beyond the removed variable.

Tested with the mentioned hotloading method and confirmed it to be working:

ERROR: JavaScript error: simulation/components/AlertRaiser.js line 141
Error: Component has no constructor
  @simulation/components/AlertRaiser.js:141:1

and

ERROR: JavaScript error: simulation/components/AlertRaiser.js line 141
Error: Failed to get property 'prototype'
  @simulation/components/AlertRaiser.js:141:1

Also changing unrecognised to unrecognized, and adding spaces between the + operator and operated values.

source/simulation2/system/ComponentManager.cpp
269 ↗(On Diff #8893)

Agree with ReportError in this case for consistency.

This revision is now accepted and ready to land.Jul 14 2019, 4:05 PM
This revision was automatically updated to reflect the committed changes.
elexis added inline comments.Sat, Jul 20, 1:56 AM
ps/trunk/source/simulation2/system/ComponentManager.cpp
173

Careful when constructing strings like this, because the temporary std::string constructed here is destroyed after leaving the scope, and with it it's c_str() pointer goes. I think it's not an issue in this call because the JS_ReportError call occurs immediately (I had tested it and it displayed fine), before the scope is left, but in other places (for example throw PSERROR_FOO(("foo" + filename + " bar").c_str()) in D2089) it won't work because the scope is left, intermediary string destroyed, and then the c_str pointer that was passed around being read from, returning random data.