See #4095. The file CNetTurnManager.cpp and the classnames CNetLocalTurnManager, CNetReplayTurnManager misleadingly connote involvement of networking code.
It were cleaner to move every class to a separate file.
The attached patch by echotangoecho transfers the server and client turnmanager to source/network/ while sorting the interface and offline turn managers to source/simulation2/system/.
Details
Details
- Reviewers
• leper elexis - Commits
- rP19165: Split TurnManager classes into individual files per class. Patch by…
- Trac Tickets
- #4095
The functionality of the networked turn managers are partially verified by test_Net.h. Since the networked turn managers are based on the local (simulation) turn manager, these are tested implicitly.
A blackbox test would include playing a singleplayer game, replaying a game, starting a server and joining with two clients, rejoining that, starting two rejoins simultaneously and rejoining while the game is paused.
Whitebox testing is more likely to find bugs by identifying and manually testing edge cases following a thorough review.
Diff Detail
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
Comment Actions
Build has FAILED
Build (release)...
../../../source/gui/CChart.cpp:38:41: warning: unused parameter ‘Message’ [-Wunused-parameter]
void CChart::HandleMessage(SGUIMessage& Message)
^
../../../source/lib/tex/tex_png.cpp: In member function ‘virtual Status TexCodecPng::encode(Tex*, DynArray*) const’:
../../../source/lib/tex/tex_png.cpp:309:9: warning: variable ‘ret’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Wclobbered]
Status ret = ERR::FAIL;
^
Build (debug)...
../../../source/gui/CChart.cpp:38:41: warning: unused parameter ‘Message’ [-Wunused-parameter]
void CChart::HandleMessage(SGUIMessage& Message)
^
Running debug tests...
Build (release)...
../../../source/simulation2/components/CCmpUnitMotion.cpp:350:45: warning: unused parameter ‘paramNode’ [-Wunused-parameter]
virtual void Deserialize(const CParamNode& paramNode, IDeserializer& deserialize)
^
../../../source/simulation2/components/CCmpUnitMotion.cpp:350:71: warning: unused parameter ‘deserialize’ [-Wunused-parameter]
virtual void Deserialize(const CParamNode& paramNode, IDeserializer& deserialize)
^
../../../source/simulation2/components/CCmpUnitMotion.cpp: In instantiation of ‘void CCmpUnitMotion::SerializeCommon(S&) [with S = ISerializer]’:
../../../source/simulation2/components/CCmpUnitMotion.cpp:347:28: required from here
../../../source/simulation2/components/CCmpUnitMotion.cpp:314:26: warning: unused parameter ‘serialize’ [-Wunused-parameter]
void SerializeCommon(S& serialize)
^
Build (debug)...
../../../source/simulation2/components/CCmpUnitMotion.cpp:350:45: warning: unused parameter ‘paramNode’ [-Wunused-parameter]
virtual void Deserialize(const CParamNode& paramNode, IDeserializer& deserialize)
^
../../../source/simulation2/components/CCmpUnitMotion.cpp:350:71: warning: unused parameter ‘deserialize’ [-Wunused-parameter]
virtual void Deserialize(const CParamNode& paramNode, IDeserializer& deserialize)
^
../../../source/simulation2/components/CCmpUnitMotion.cpp: In instantiation of ‘void CCmpUnitMotion::SerializeCommon(S&) [with S = ISerializer]’:
../../../source/simulation2/components/CCmpUnitMotion.cpp:347:28: required from here
../../../source/simulation2/components/CCmpUnitMotion.cpp:314:26: warning: unused parameter ‘serialize’ [-Wunused-parameter]
void SerializeCommon(S& serialize)
^
Running debug tests...
./test_dbg: error while loading shared libraries: libmozjs38-ps-debug.so: cannot open shared object file: No such file or directory
Build (release)...
In file included from ../../../source/simulation2/components/CCmpPathfinder_Common.h:39:0,
from ../../../source/simulation2/components/CCmpPathfinder.cpp:25:
../../../source/simulation2/helpers/LongPathfinder.h:266:15: error: ‘mutex’ in namespace ‘std’ does not name a type
mutable std::mutex m_Mutex;
^
make[1]: *** [obj/simulation2_Release/CCmpPathfinder.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [simulation2] Error 2
make: *** Waiting for unfinished jobs....
Build (release)...
Build (debug)...
Running debug tests...
./test_dbg: error while loading shared libraries: libmozjs38-ps-debug.so: cannot open shared object file: No such file or directoryLink to build: http://jw:8080/job/phabricator/24/
See console output for more information: http://jw:8080/job/phabricator/24/console
Comment Actions
Looks sane. I guess most of the issues can be fixed later, so I don't see an issue with including this once we have a passing build.
| source/network/NetClientTurnManager.cpp | ||
|---|---|---|
| 31 ↗ | (On Diff #45) | All supported compilers should support variadic macros (else the logging code would be broken), so use that and make all uses of this nicer. |
| 130 ↗ | (On Diff #45) | Does this add a trailing comma? |
| 139 ↗ | (On Diff #45) | This should most likely be translated, so maybe just push the data to the gui and let that format everything? |
| source/network/NetClientTurnManager.h | ||
| 29 ↗ | (On Diff #45) | NONCOPYABLE? |
| source/network/NetServerTurnManager.cpp | ||
| 27 ↗ | (On Diff #45) | __VA_ARGS__ |
| 32 ↗ | (On Diff #45) | IIRC we have the : in the next line. |
| source/ps/Util.h | ||
| 21 ↗ | (On Diff #45) | Why is this here? |
| source/simulation2/system/LocalTurnManager.cpp | ||
| 22 ↗ | (On Diff #45) | \n : |
| source/simulation2/system/ReplayTurnManager.cpp | ||
| 26 ↗ | (On Diff #45) | \n : |
| 56 ↗ | (On Diff #45) | Could be const if one would use the iterator we get from find instead of doing two more lookups. |
| source/simulation2/system/TurnManager.cpp | ||
| 36 ↗ | (On Diff #45) | __VA_ARGS__ though by now I see that this was only moved, so maybe fix that in another commit. |
| 70 ↗ | (On Diff #45) | Could be const |
| 72 ↗ | (On Diff #45) | Do we have a few tests for this? Should this also be the case for UpdateFastForward? |
| 253 ↗ | (On Diff #45) | const? |
Comment Actions
Retry upload. Use const ref, Broadcast for kick information messages, merge two If statements.
Comment Actions
Build is green
Updating workspaces.
Build (release)...
../../../source/gui/CChart.cpp:38:41: warning: unused parameter ‘Message’ [-Wunused-parameter]
void CChart::HandleMessage(SGUIMessage& Message)
^
../../../source/lib/tex/tex_png.cpp: In member function ‘virtual Status TexCodecPng::encode(Tex*, DynArray*) const’:
../../../source/lib/tex/tex_png.cpp:309:9: warning: variable ‘ret’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Wclobbered]
Status ret = ERR::FAIL;
^
Build (debug)...
../../../source/gui/CChart.cpp:38:41: warning: unused parameter ‘Message’ [-Wunused-parameter]
void CChart::HandleMessage(SGUIMessage& Message)
^
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!http://jw:8080/job/phabricator/55/ for more details.
Comment Actions
Build has FAILED
Link to build: http://jw:8080/job/phabricator/56/
See console output for more information: http://jw:8080/job/phabricator/56/console
Comment Actions
Updates by echotangoecho:
Variadic macros.
Const keyword for WillUpdate and TurnNeedsFullHash.
Override specifier to ensure that the function has the virtual keyword in the base class.
NONCOPYABLE for the NetClientTurnManager to make it more transparent (as the base class is already NONCOPYABLE).
Explicit type instead of the auto specifier.
2016 -> 2017.
Move constructor colon to the next line.
https://github.com/echotangoecho/0ad/commit/96a2cfe3f06a657ed5b63b57aab41f55370848e2
https://github.com/echotangoecho/0ad/commit/e1181ecedabb8a2140ca795977b2736e9725edfe
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/127/ for more details.
| source/network/NetClientTurnManager.cpp | ||
|---|---|---|
| 31 ↗ | (On Diff #45) | Implemented. |
| 130 ↗ | (On Diff #45) | No, since the stream ends with a playername in each step and since the first comma is skipped. |
| 139 ↗ | (On Diff #45) | Agree, but in a separate revision. |
| source/network/NetClientTurnManager.h | ||
| 29 ↗ | (On Diff #45) | Was added for transparency, but it is optional: 14:03 < echotangoecho> if we inherit from a noncopyable class, doesn't the subclass become noncopyable as well? (don't know anything about this) 14:05 < echotangoecho> that suggests that it becomes noncopyable as well, right? 14:05 < echotangoecho> if the copy constructor of the base is deleted we can't copy the subclass either 14:11 < echotangoecho> but yes, we could add it 14:21 < echotangoecho> it can't be copied at all 14:21 < echotangoecho> as the base class can't be copied 14:21 < echotangoecho> so trying to do that would yield a compilation error |
| source/network/NetServerTurnManager.cpp | ||
| 27 ↗ | (On Diff #45) | Implemented. |
| 32 ↗ | (On Diff #45) | Yes we do. Implemented. |
| source/ps/Util.h | ||
| 21 ↗ | (On Diff #45) | Because Util.h contains the void createDateIndexSubdirectory(const OsPath& parentDir) refering to an OsPath and since that include was not needed in Util.h, as NetTurnManager.h already covered it after rP16727 added it when the OOS error GUI message was extended and moved to a new void DisplayOOSError which had an OsPath argument, hence introduced an OsPath to the header file of the NetTurnManager. The patch moves the #include from NetTurnManager.h to Util.h, as Util.cpp contains the method createDateIndexSubdirectory that returns an OsPath. Might be interchangeable. |
| source/simulation2/system/ReplayTurnManager.cpp | ||
| 56 ↗ | (On Diff #45) | Doesn't look like the function can become const since it calls nonconst functions. |
| source/simulation2/system/TurnManager.cpp | ||
| 36 ↗ | (On Diff #45) | Implemented in the same patch. |
| 70 ↗ | (On Diff #45) | Done. |
| 72 ↗ | (On Diff #45) | test_Net.h doesn't include any tests that aren't DISABLED and wouldn't fail in many ways if enabled. For example test_basic_DISABLED yields: .The commit will be easier to review/audit if we don't fix all of these things in the same go, since right now, the code is to basically 95% of the previous code, especially no logic changes. So agreeing to restore network test abilities in a separate ticket. |
| 253 ↗ | (On Diff #45) | Done. |
Comment Actions
@Itms can you set echotangoecho as the author of the differential and me as a reviewer, once he got an account?
Comment Actions
The comment inaccuracy and the override comments are teh only ones that really matter and should be addressed, apart from that I'm ok with this.
| source/network/NetClientTurnManager.cpp | ||
|---|---|---|
| 130 ↗ | (On Diff #45) | Ah, indeed. Sorry for the noise. |
| source/ps/Util.h | ||
| 21 ↗ | (On Diff #45) | (Should have looked at more context when I asked.) It is only needed because createDateIndexSubdirectory returns OsPath, though that means the previous code assumed that clients got the include order right, so that change seems good. |
| source/simulation2/system/LocalTurnManager.h | ||
| 31 ↗ | (On Diff #179) | Either someone forgot to update http://trac.wildfiregames.com/wiki/CppSupport (is there a shorter way to reference the trac wiki than just posting the full link?) and discuss the reason for dropping GCC4.6, or this shouldn't be here (or elsewhere as there are quite a few below). (Not that we couldn't just #define override in some header and make it work that way, but that sounds bad IMO. Also I guess the build bot is using GCC4.8 or something since I guess it is using the next Ubuntu LTS which IIRC has that as the default.) |
| source/simulation2/system/ReplayTurnManager.cpp | ||
| 72 ↗ | (On Diff #179) | This, and other cases like it, might want to use decomposition declarations (aka structured bindings) for that once we have C++17. |
| 56 ↗ | (On Diff #45) | I seem to have missed DoTurn which sounds very non-const when I wrote that. |
| source/simulation2/system/TurnManager.cpp | ||
| 72 ↗ | (On Diff #45) | Agreed. |
| source/simulation2/system/TurnManager.h | ||
| 37 ↗ | (On Diff #179) | This is slightly inaccurate now that we've moved most of the code this refers to elsewhere. Would "the (network) turn system" be better? (Not sure.) |
| source/simulation2/system/LocalTurnManager.h | ||
|---|---|---|
| 31 ↗ | (On Diff #179) | According to echotangoecho, ricotz said that sm38 doesn't build with gcc 4.6. No SM38 document I could find explicitly ruled out gcc 4.6 incomplete draft of the sm38 release announcement (SpiderMonkey 38 is the JavaScript engine that shipped in Firefox 38.). Firefox 38 dropped gcc 4.6. though: echotangoecho kindly updated our wiki page in version 4 |
| source/simulation2/system/LocalTurnManager.h | ||
|---|---|---|
| 31 ↗ | (On Diff #179) | Can I have one more indirection for that information please? Quite possible that they dropped it, but that should have been discussed properly and been a conscious (and easy) decision instead of "Oops, we broke the build". Also given that the main reason for GCC4.6 was/is Ubuntu 12.04 (which is EoL in roughly 3 months), we could look at what the other important (and outdated) distros are. From a quick look both Ubuntu 14.04 and Debian stable ship 4.8 or higher, so we could just go to that and have one compiler with full C++11 support. (And someone should most likely look at the most recent clang version shipped for OS X, since that is most likely also providing complete support.) Which might then lead to someone to doing something with the VS2015 patches we have in multiple places. Also adding another link instead of fixing the first one that redirects to it... (That release announcement is not really incomplete, that's a lot better than a few of the others already...) |
Comment Actions
I wouldn't mind going to GCC 4.8, but Debian 7 only has GCC 4.7, so the question is wether we care enough about that to keep supporting it (personally, I don't think preserving support for these older distros is that useful, as it usually isn't difficult to upgrade them and so not many people will be using them, contrary to older windows releases, which are luckily supported by newer compilers on that platform).
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/226/ for more details.
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/229/ for more details.
Comment Actions
Debian 7 is oldstable, which even with backports only has Alpha 17. It might still get LTS support for a year and a half, but nobody running oldstable should expect to have recent software support them. I did start an (internal) discussion about dropping a few old compilers which so far seems to go quite well.
@elexis Differing opionion? (I'll let you do the committing)
| source/simulation2/system/TurnManager.h | ||
|---|---|---|
| 37 ↗ | (On Diff #179) | s/on/for/? |
Comment Actions
If those are all distributions we support(ed), then I agree that we can drop GCC 4.7 too. Thanks for the branch and proper review!
| source/simulation2/system/TurnManager.h | ||
|---|---|---|
| 70 ↗ | (On Diff #179) | That comment should be m_PlayerID instead. |
| 174 ↗ | (On Diff #179) | These two members should have a comment too ideally. m_ClientID is a sequential ID determined by the server, transmitted via the CAuthenticateResultMessage. It seems weird that we have the clientID in this class in the first place. It is 0 for the local and replay turnmanager, the server turnmanager doens't inherit this class, thus not this property. Only the client turnmanager needs it, and it doesn't need it currently but only once we do #3923. Guess we have to keep it since we don't want to change AddCommand in a weird way (like making that argument optional). |
Comment Actions
That was meant to refer to accepting this revision/you doing the same. For the compiler discussion see the forum topic.