Pass ScriptInterface as a const ref where possible.
ClosedPublic
Actions

Authored by • leper on Jul 12 2017, 11:34 PM.

Details

Reviewers

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Owners Package	(Owns No Changed Paths)

Commits

rP20028: Pass ScriptInterface as a const ref where possible.

Summary

Make a few functions of it const so this works.
(A few of the serialization functions might not depend on the constification of ScriptInterface, but splitting them wouldn't make a lot of sense.)

Adding @echotangoecho to the loop since D492 touches some of the same filse.

Test Plan

Check if it compiles and runs, possibly read the code

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

• leper created this revision.Jul 12 2017, 11:34 PM

Owners added a subscriber: Restricted Owners Package.Jul 12 2017, 11:34 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1736/ for more details.

Executing section Default...
Executing section Source...

source/gui/scripting/ScriptFunctions.cpp
| 711| »   return·*(volatile·int*)0;
|    | [MAJOR] CPPCheckBear (nullPointer):
|    | Null pointer dereference
Executing section JS...
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/309/ for more details.

Harbormaster completed remote builds in B2594: Diff 2898.Jul 13 2017, 1:09 AM

• leper mentioned this in D492: Cleanup of the FSM and NetMessage code in network..Jul 30 2017, 9:48 PM

After the MessageTypeConversions.cpp merge fix, it compiles and works on Windows 8.1. Only question, why always const, but there are functions, which look like the changing of the script interface?

source/i18n/scripting/JSInterface_L10n.cpp
160 ↗	(On Diff #2898)	Why the `scriptInterface` is const and we change (may think that we change) its state below? It looks weird.

• leper mentioned this in rP20019: Remove unused Renderer function from rP13877..Aug 21 2017, 9:41 PM

Only question, why always const, but there are functions, which look like the changing of the script interface?

Because we can, and because it might point out some cases where we are likely to do stupid things in the future. Some functions are indeed strange as const, since they do seem to change internal state, but that is in SpiderMonkey and not the ScriptInterface, so at that point we're mostly doing it for consistency and because we can.

source/i18n/scripting/JSInterface_L10n.cpp
160 ↗	(On Diff #2898)	Because RegisterFunction doesn't actually change any member of ScriptInterface, mostly because of how SpiderMonkey works.

Good patch.

Approach: The script interface refs passed around should indeed be const, so that callers don't break it unintentionally.
Correctness: Tested the patch after rebasing, passes the compilation with gcc 5.4.1.
Completeness: Can be comimitted as is (certainly better than letting this patch get outdated).

With grep -Ri 'scriptInterface&' * | grep -v 'const ScriptInterface&' we can find few more candidates:

gui/scripting/JSInterface_IGUIObject.cpp:void JSI_IGUIObject::init(ScriptInterface& scriptInterface)
gui/scripting/ScriptFunctions.h:void GuiScriptingInit(ScriptInterface& scriptInterface);
gui/scripting/JSInterface_GUITypes.h:	void init(ScriptInterface& scriptInterface);
gui/scripting/ScriptFunctions.cpp:void GuiScriptingInit(ScriptInterface& scriptInterface)
gui/scripting/JSInterface_IGUIObject.h:	void init(ScriptInterface& scriptInterface);
gui/scripting/JSInterface_GUITypes.cpp:void JSI_GUITypes::init(ScriptInterface& scriptInterface)
lobby/XmppClient.h:	ScriptInterface& GetScriptInterface();
network/NetClient.h:	ScriptInterface& GetScriptInterface();
network/NetClient.h:	void SendGameSetupMessage(JS::MutableHandleValue attrs, ScriptInterface& scriptInterface);
network/NetServer.cpp:ScriptInterface& CNetServerWorker::GetScriptInterface()
network/NetServer.cpp:		ScriptInterface& scriptInterface = server.GetScriptInterface();
network/NetServer.cpp:void CNetServer::UpdateGameAttributes(JS::MutableHandleValue attrs, ScriptInterface& scriptInterface)
network/NetClientTurnManager.cpp:	ScriptInterface& scriptInterface = m_NetClient.GetScriptInterface();
network/NetServer.h:	void UpdateGameAttributes(JS::MutableHandleValue attrs, ScriptInterface& scriptInterface);
network/NetServer.h:	ScriptInterface& GetScriptInterface();
network/NetClient.cpp:ScriptInterface& CNetClient::GetScriptInterface()
network/NetClient.cpp:void CNetClient::SendGameSetupMessage(JS::MutableHandleValue attrs, ScriptInterface& scriptInterface)
ps/GameSetup/GameSetup.cpp:	ScriptInterface& scriptInterface = g_Game->GetSimulation2()->GetScriptInterface();
ps/GameSetup/GameSetup.cpp:	ScriptInterface& scriptInterface = g_Game->GetSimulation2()->GetScriptInterface();
ps/Game.cpp:	ScriptInterface& scriptInterface = m_Simulation2->GetScriptInterface();
ps/Game.cpp:	ScriptInterface& scriptInterface = m_Simulation2->GetScriptInterface();
ps/TemplateLoader.cpp:std::vector<std::string> CTemplateLoader::FindPlaceableTemplates(const std::string& path, bool includeSubdirectories, ETemplatesType templatesType, ScriptInterface& scriptInterface) const
ps/TemplateLoader.h:	std::vector<std::string> FindPlaceableTemplates(const std::string& path, bool includeSubdirectories, ETemplatesType templatesType, ScriptInterface& scriptInterface) const;
simulation2/system/SimContext.h:	ScriptInterface& GetScriptInterface() const;
simulation2/system/ComponentManager.h:	ScriptInterface& GetScriptInterface() { return m_ScriptInterface; }
simulation2/system/InterfaceScripted.h:	void ICmp##iname::InterfaceInit(ScriptInterface& scriptInterface) { \
simulation2/system/InterfaceScripted.h:	void RegisterComponentInterface_##iname(ScriptInterface& scriptInterface) { \
simulation2/system/ComponentTest.h:	ScriptInterface& GetScriptInterface()
simulation2/system/SimContext.cpp:ScriptInterface& CSimContext::GetScriptInterface() const
simulation2/system/ComponentManager.cpp:	extern void RegisterComponentInterface_##name(ScriptInterface&); \
simulation2/system/Interface.h:	static void InterfaceInit(ScriptInterface& scriptInterface); \
simulation2/Simulation2.cpp:	ScriptInterface& scriptInterface = m_ComponentManager.GetScriptInterface();
simulation2/Simulation2.cpp:ScriptInterface& CSimulation2::GetScriptInterface() const
simulation2/Simulation2.cpp:	ScriptInterface& scriptInterface = GetScriptInterface();
simulation2/Simulation2.h:	ScriptInterface& GetScriptInterface() const;
simulation2/components/CCmpTemplateManager.cpp:	ScriptInterface& scriptInterface = this->GetSimContext().GetScriptInterface();
simulation2/components/CCmpCommandQueue.cpp:		ScriptInterface& scriptInterface = GetSimContext().GetScriptInterface();
simulation2/components/CCmpAIManager.cpp:		ScriptInterface& scriptInterface = GetSimContext().GetScriptInterface();
simulation2/components/CCmpAIManager.cpp:		ScriptInterface& scriptInterface = GetSimContext().GetScriptInterface();
simulation2/components/CCmpAIManager.cpp:		ScriptInterface& scriptInterface = GetSimContext().GetScriptInterface();
simulation2/components/CCmpAIManager.cpp:		ScriptInterface& scriptInterface = GetSimContext().GetScriptInterface();
simulation2/components/CCmpAIManager.cpp:		ScriptInterface& scriptInterface = GetSimContext().GetScriptInterface();
tools/atlas/GameInterface/Handlers/MapHandlers.cpp:		ScriptInterface& scriptInterface = g_Game->GetSimulation2()->GetScriptInterface();
tools/atlas/GameInterface/Handlers/MapHandlers.cpp:		ScriptInterface& scriptInterface = g_Game->GetSimulation2()->GetScriptInterface();
tools/atlas/GameInterface/Handlers/MapHandlers.cpp:	ScriptInterface& scriptInterface = g_Game->GetSimulation2()->GetScriptInterface();

The one in the XmppClient is unused as you said and I could confirm that it can be nuked (rP14098). Could be added to the patch and mentioned in the commit message or not.
Considering that you knew about the XmppClient one, I guess there are more patches where this one came from. :-)

source/renderer/scripting/JSInterface_Renderer.cpp
68 ↗	(On Diff #2898)	needs rebase
source/renderer/scripting/JSInterface_Renderer.h
49 ↗	(On Diff #2898)	needs rebase
source/simulation2/scripting/MessageTypeConversions.cpp
55 ↗	(On Diff #2898)	entire file needs rebase
source/test_setup.cpp
149 ↗	(On Diff #2898)	good rename. `ok` could be inlined if one wants to.

This revision is now accepted and ready to land.Aug 21 2017, 11:40 PM

With grep -Ri 'scriptInterface&' * | grep -v 'const ScriptInterface&' we can find few more candidates:

At least all those GUI init ones cannot be made const, since one of the functions they use isn't const, and cannot be made const.

For the others I think I tried making most of them const and they are either used by something else or cannot be const. I guess I can give all of that a try once this thing is merged.

The one in the XmppClient is unused as you said and I could confirm that it can be nuked (rP14098). Could be added to the patch and mentioned in the commit message or not.
Considering that you knew about the XmppClient one, I guess there are more patches where this one came from. :-)

Well that diff had 2 files, and one of those was committed already. So in case that is referring to the specific code in question most likely not (see above for exceptions), apart from that I might have a few more diffs around, though quite a few of those aren't in a state that'd be worth submitting. (See e.g. the nvtt ticket/branch that's around; the build is working, but there is some cleanup, making sure it works on all platforms, etc missing)

Thanks for the review!

Closed by commit rP20028: Pass ScriptInterface as a const ref where possible. (authored by • leper). · Explain WhyAug 24 2017, 2:32 AM

This revision was automatically updated to reflect the committed changes.

With grep -Ri 'scriptInterface&' * | grep -v 'const ScriptInterface&' we can find few more candidates:
[list]

The gui/ ones are needed since those functions cannot be const (some SpiderMonkey function not able to deal with that, which makes sense for those).
The component registration ones have the same issue IIRC.
Fixed the template loading ones locally, and I initially didn't consider non parameter ones, but changing those now that there isn't much left seems useful. Already found another unused function (GetGlobalClass).