Details
Make sure that every pointer change is actually correct, make sure that every leak is identified and addressed (not only the reported ones).
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 8432 Build 13776: Vulcan Build Jenkins Build 13775: arc lint + arc unit
Event Timeline
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/lobby/glooxwrapper/glooxwrapper.h | 88| namespace·glooxwrapper | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'namespaceglooxwrapper{' is invalid C code. Use --std or --language to configure the language. source/lobby/XmppClient.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/120/display/redirect
See https://code.wildfiregames.com/rP19703#35141 for a comment on every line.
Fix identified and previously unidentified leaks.
Use references in many places.
Supersede c-style casts with static_cast and reinterpret_cast.
Remove unused classes.
Remove default arguments from implementation and especially headers.
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/network/NetClient.h | 59| class·CNetClient·:·public·CFsm | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'classCNetClient:' is invalid C code. Use --std or --language to configure the language. source/lobby/XmppClient.h | 24| | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'namespaceStunClient{' is invalid C code. Use --std or --language to configure the language. source/network/StunClient.h | 26| namespace·StunClient | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'namespaceStunClient{' is invalid C code. Use --std or --language to configure the language. source/lobby/glooxwrapper/glooxwrapper.h | 88| namespace·glooxwrapper | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'namespaceglooxwrapper{' is invalid C code. Use --std or --language to configure the language. source/lobby/IXmppClient.h | 24| namespace·StunClient·{ | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'namespaceStunClient{' is invalid C code. Use --std or --language to configure the language. source/network/tests/test_Net.h | 37| class·TestNetComms·:·public·CxxTest::TestSuite | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'classTestNetComms:' is invalid C code. Use --std or --language to configure the language. Executing section JS... | | [NORMAL] ESLintBear (indent): | | Expected indentation of 2 tabs but found 3. |----| | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js | |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js | 210| 210| error("Unrecognised net message type: " + message.type); | 211| 211| } | 212| 212| else | 213| |- // Not rejoining - just trying to connect to server | | 213|+ // Not rejoining - just trying to connect to server | 214| 214| | 215| 215| switch (message.type) | 216| 216| { Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/132/display/redirect
source/lobby/XmppClient.cpp | ||
---|---|---|
1221 | (handleSessionInitiation sounds a lot like it has legitimate access to session, hence the UNUSED moved from handleSessionAction) | |
source/lobby/glooxwrapper/glooxwrapper.cpp | ||
813 | Leak / Unused code: If the unused class wasn't removed (I did that too in D2094), it would probably be cleaner to return references, or tell the caller to delete the plugins. | |
856 | Unused code / Leak: There would have been a destructor nuking m_Wrapped missing here. | |
894 | ||
919 | inline glooxSession, otherwise double-free is redundant (can be done when committing) | |
source/lobby/glooxwrapper/glooxwrapper.h | ||
642 | This was the unused variable from D2090 fixed by the ~Session conditionally deleting m_Wrapped. |
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/network/StunClient.h | 26| namespace·StunClient | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'namespaceStunClient{' is invalid C code. Use --std or --language to configure the language. source/lobby/IXmppClient.h | 24| namespace·StunClient·{ | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'namespaceStunClient{' is invalid C code. Use --std or --language to configure the language. source/network/tests/test_Net.h | 37| class·TestNetComms·:·public·CxxTest::TestSuite | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'classTestNetComms:' is invalid C code. Use --std or --language to configure the language. source/lobby/glooxwrapper/glooxwrapper.h | 88| namespace·glooxwrapper | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'namespaceglooxwrapper{' is invalid C code. Use --std or --language to configure the language. source/network/NetClient.h | 59| class·CNetClient·:·public·CFsm | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'classCNetClient:' is invalid C code. Use --std or --language to configure the language. source/lobby/XmppClient.h | 24| | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'namespaceStunClient{' is invalid C code. Use --std or --language to configure the language. Executing section JS... | | [NORMAL] ESLintBear (indent): | | Expected indentation of 2 tabs but found 3. |----| | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js | |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js | 210| 210| error("Unrecognised net message type: " + message.type); | 211| 211| } | 212| 212| else | 213| |- // Not rejoining - just trying to connect to server | | 213|+ // Not rejoining - just trying to connect to server | 214| 214| | 215| 215| switch (message.type) | 216| 216| { Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/139/display/redirect
source/lobby/glooxwrapper/glooxwrapper.cpp | ||
---|---|---|
295 | (nitpick rename because m_Wrapped relates to the item wrapped while these variables refer to the wrapper, also "dromedary case" / lower-camel-case) |
I'm not entirely sure that all these changes are needed, but they seem okay. Coming from mostly writing C, it strikes me as particularly strange to modify something passed as a C++ reference. However, it doesn't seem that there's anything technically wrong with that. I assume you've tested that hosting a game and STUN still work?
source/lobby/XmppClient.cpp | ||
---|---|---|
1201 | const stunEndpoint? |
Thank you for the review @JoshuaJB !
Also thanks to @fcxSanya (original author) who didn't have time for a technical review but still responded with a general encouragement / appreciation answer.
About the changes being needed:
- There is a clang compiler warning about a glooxwrapper thing being unused is an actually occurring leak.
- The two unused classes miss a destructor, deleting them altogether makes the code easier to maintain.
- Using references means the caller doesnt have to worry anymore about whether or not the thing has to be deleted.
Coming from mostly writing C, it strikes me as particularly strange to modify something passed as a C++ reference. However, it doesn't seem that there's anything technically wrong with that.
I talked to @vladislavbelov on IRC about pointer vs reference briefly on http://irclogs.wildfiregames.com/2019-08/2019-08-14-QuakeNet-%230ad-dev.log
21:53 < Vladislav> It depends, I and some known CC recommends to use only const ref for read and pointers for read/write. Because it might be easier to understand that a function may change an argument: foo(arg) or foo(&arg).
I assume you've tested that hosting a game and STUN still work?
Of course. Not only hosting, but also joining, otherwise it's a bit inconclusive :-p
I also ran valgrind on it and neither xmpp nor gloox appears in the log.
source/lobby/XmppClient.cpp | ||
---|---|---|
1201 | ack |