The current code for the FSM and the handling (serialization, deserialization) of NetMessages is dated and not very nice. This patch tries to improve both.
Details
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- netmessages
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 1795 Build 2841: Vulcan Build Jenkins Build 2840: arc lint + arc unit
Event Timeline
source/network/NetClient.cpp | ||
---|---|---|
405 | I suspect that we might want to enable writing that on one line again. | |
865 | switch | |
source/network/NetFileTransfer.cpp | ||
29 | switch | |
source/network/NetFileTransfer.h | ||
21 | Just forward decl it instead. | |
source/network/NetMessageBuffer.cpp | ||
63 | These three could just be in the header. | |
103 | Why the first param? | |
source/network/NetMessageBuffer.h | ||
56 | Where do we need the non-const one? | |
57 | That comment seems pointless. | |
187 | At least follow the example in the simulation serialization code and fail explicitly in case anyone ever tries to build on something big endian. | |
231 | Same here. | |
source/network/NetServer.cpp | ||
1026 | switch? | |
source/network/tests/test_NetMessage.h | ||
110 | This seems to slightly miss the point of testing msg2 against a known good string. | |
source/ps/CStr.cpp | ||
31 | is this one still needed? | |
source/ps/tests/test_CStr.h | ||
109 | If you remove these I'd have expecte something similar in the new tests. |
source/network/NetMessageBuffer.cpp | ||
---|---|---|
103 | It is required for the CBinarySerializer, but I don't need it here. Guess I could use UNUSED() here. | |
source/network/NetMessageBuffer.h | ||
187 | I don't think this code depends on endianness? I just serialize the integer to little-endian here, and deserialize it as such. | |
source/ps/tests/test_CStr.h | ||
109 | The serialization / deserialization of strings is already tested in the tests for net messages. |
source/network/NetMessageBuffer.h | ||
---|---|---|
56 | The non-const version is used during deserialization (the members are passed by non-const reference to the deserialization functions). | |
source/network/tests/test_NetMessage.h | ||
110 | The idea here was to only test whether the serialization / deserialization of the messages works correctly, the actual JS serialization tests are (should) already be done in simulation2/tests/test_Serializer.h. |
Build has FAILED
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (308 tests)...........................................................................................................................................................................................................................................Assertion failure: initialized(), at ../../../libraries/source/spidermonkey/include-unix-debug/js/RootingAPI.h:1161 Segmentation fault
Link to build: http://jw:8080/job/phabricator/1247/
See console output for more information: http://jw:8080/job/phabricator/1247/console
A few more comments.
source/network/Fsm.cpp | ||
---|---|---|
21 | At least some of the codebase has \n\t: m_Foo(bar). | |
40 | I do hope that we do take quite some care (as in fail-safe or rather fail-early-and-loud checks) that the lifetime of that buffer actually is longer than that of the event. | |
41 | Actually this whole implementation is that short (and mostly trivial) that it could just be done in the header, but I guess that is just personal preference. | |
64 | These sound just like header defs, but well. | |
71 | Unless we expect this to handle multiple actions (which would make me question the function name), why not just result = ...? Or even return !m_Action.m_Function || m_Action.m_Function(...)? (The latter might be golfing, but the &= seems pointless. | |
89 | Could use emplace, given that we are really just calling the ctor and passing that to push_back. | |
114 | Guess what ;-) | |
169 | Yeah, well. | |
source/network/Fsm.h | ||
89 | Is anything actually inheriting from this? | |
source/network/NetMessageBuffer.cpp | ||
103 | UNUSED would make make this obvious. | |
105 | const? | |
source/network/NetMessageBuffer.h | ||
18 | You might want to update that for the new filename. | |
127 | Are there many occurences where we need a copy of this? | |
135 | Could be const | |
187 | Ah, right. Sorry for the noise. | |
386 | I do like the comments what #if this closes, especially if there is most of the file in-between. | |
source/network/NetServer.h | ||
239 | I might be missing something, but can't event be a reference? | |
source/network/tests/test_NetMessage.h | ||
36 | This, | |
45 | and this seem useful though. |
- Fix assert failures in debug mode (add default constructor to SerializeableJSValue).
- Remove unneeded copy constructor for CSimulationMessage.
- Fix most (all?) issues found by leper.
source/network/Fsm.cpp | ||
---|---|---|
21 | It is the questions which we prefer: looking in some of the simulation2 and the network files, there is no break before the :. | |
40 | The buffer is only used from inside the update function where it is used to execute all handlers, and they probably shouldn't depend on the buffer living any longer than for the execution of the function. | |
source/network/NetHost.cpp | ||
55 | The check is moved to the GetData function on the NetMessageBuffer, but it probably still isn't really needed. | |
source/network/NetMessageBuffer.h | ||
127 | only in one place, I have added a data() and size() function for that. |
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jw:8080/job/phabricator/1703/ for more details.
CFooBarMessage message; event.GetMessageBuffer().GetMessage(message);
seems like a quite common piece of code that could possibly be handled within the constructor. (It might not be nicer, but I'd like to know why in that case.)
Somewhat related to this patch but I do have another const diff for ScriptInterface lying around, which I guess will only mean replacing ScriptInterface& by const ScriptInterface& depending on what is merged first.
There are still 2 comments (one marked TODO, the other one being about what scriptinterface to use IIRC) that should probably be fixed.
source/network/Fsm.cpp | ||
---|---|---|
21 | I somewhat dislike having that thing trailing at the end of a long line like that, but if you prefer this, fine by me. | |
65 | That was what that one emplace_back comment below referred to IIRC. | |
source/network/NetClient.cpp | ||
72–75 | nullptr? | |
77–79 | I suspect we should rename that member variable to be consistent with all other uses of GetContext. | |
source/network/NetFileTransfer.cpp | ||
29 | \n{ | |
source/network/NetMessageBuffer.cpp | ||
106 | Could this be const std::vector<u8>&? (Would also make the few users of this use the same type, avoiding the need to copy.) |
- Replace some NULL occurrences with nullptr.
- push_back -> emplace_back in some places.
- Fix some more style issues (including some whitespace things).
- Remove leftover TODO and comment about scriptInterface.
- Add an easier syntax for deseralizing a message, as suggested by leper.
source/network/NetClient.cpp | ||
---|---|---|
77–79 | Might be something for a later patch? |
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jw:8080/job/phabricator/1769/ for more details.
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jw:8080/job/phabricator/1770/ for more details.
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jw:8080/job/phabricator/1771/ for more details.
This is looking good so far, I guess this should get quite extensive testing on as many platforms/compilers as we can get.
source/network/Fsm.h | ||
---|---|---|
46 | Why not NetMessageType? | |
111 | Why aren't we using NetMessageType for eventType? (Applies to the whole class + users; mostly this would remove all the (uint) casts for AddTransition callers.) | |
source/network/NetClient.cpp | ||
77–79 | Fine by me. | |
837 | No need for the cast. | |
source/network/NetMessageBuffer.cpp | ||
91 | What if this overflows? Yes, this is unlikely to occur, but this is in network code. | |
137 | See the comment marked as done about also having users of GetBuffer use const std::vector<u8>&. | |
source/network/NetMessages.h | ||
74 | Possibly const&, if D739 gets merged first. | |
source/network/NetServer.cpp | ||
1084 | Pointless cast (just check the other users of GetType or NMT_* too). | |
source/network/NetServerTurnManager.cpp | ||
128 | Could nuke the {}. | |
source/network/NetSession.cpp | ||
145 | Why does this hunk differ in the (if (msg)) check from the one in net server? |