Differential D2090
glooxwrapper: Remove unused private 'm_Owned' in Jingle::Session::Jingle Authored by elexis on Jul 17 2019, 2:33 AM.
Details
Fix compiler warning (Apple LLVM version 10.0.1; clang-1001.0.46.4):
It appears to have been unused since its introduction in rP19703. Jenkins. Use clang to test for unused variables.
Diff Detail
Event TimelineComment Actions Build failure - The Moirai have given mortals hearts that can endure. Link to build: https://jenkins.wildfiregames.com/job/docker-differential/110/display/redirect Comment Actions Okay, it can't just be removed as-is. Apple clang fails as follows: clang In file included from ../../../source/lobby/StanzaExtensions.cpp:18:
In file included from ../../../source/lobby/StanzaExtensions.h:20:
../../../source/lobby/glooxwrapper/glooxwrapper.h:643:93: error: member initializer 'm_Owned' does not name a non-static data member or base class
Jingle(const gloox::Jingle::Session::Jingle* wrapped, bool owned) : m_Wrapped(wrapped), m_Owned(owned) {}
^~~~~~~~~~~~~~
In file included from ../../../source/lobby/XmppClient.cpp:20:
In file included from ../../../source/lobby/XmppClient.h:25:
../../../source/lobby/glooxwrapper/glooxwrapper.h:643:93: error: member initializer 'm_Owned' does not name a non-static data member or base class
Jingle(const gloox::Jingle::Session::Jingle* wrapped, bool owned) : m_Wrapped(wrapped), m_Owned(owned) {}
^~~~~~~~~~~~~~And gcc, in Jenkins, fails similarly: gcc In file included from ../../../source/lobby/StanzaExtensions.h:20:0,
from ../../../source/lobby/StanzaExtensions.cpp:18:
../../../source/lobby/glooxwrapper/glooxwrapper.h: In constructor 'glooxwrapper::Jingle::Session::Jingle::Jingle(const gloox::Jingle::Session::Jingle*, bool)':
../../../source/lobby/glooxwrapper/glooxwrapper.h:643:93: error: class 'glooxwrapper::Jingle::Session::Jingle' does not have any field named 'm_Owned'
Jingle(const gloox::Jingle::Session::Jingle* wrapped, bool owned) : m_Wrapped(wrapped), m_Owned(owned) {}
^~~~~~~
Comment Actions Causes more warnings. Needs additional changes to either accommodate the absence of this field, or additional changes to make the original version not cause warnings. Comment Actions In fact it was a leak and needed the same destructor that the other glooxwrapper classes have: glooxwrapper::Jingle::Session::~Session()
{
if (m_Owned)
delete m_Wrapped;
}The gloox::Jingle::Session is created in glooxwrapper::SessionManager::createSession, that returns the glooxwrapper::Jingle::Session, createSession is called in XmppClient::SendStunEndpointToHost. As that wrapper session is not a pointer, it will be deleted, but due to its missing destructor, it won't delete the wrapped gloox session. Subject to D2094. Comment Actions It's a hypothetical leak, if m_Owned = truewas passed in the constructor. But the only instantiation is in handleSessionAction which sets owned to false (must be because it is created and deleted by gloox after gloox calls this session handler function implemented by the XmppClient. The specs don't speak about it, but one gets a crash if one tries to delete it, and it makes sense that gloox takes care of both the Session and the Jingle). Above destructor is still the way to address the unused variable, since every glooxwrapper class should have the m_Wrapped member (the gloox class that the glooxwrapper wraps) and the m_Owned boolean that denotes whether the glooxwrapper will take ownership of the instance it wraps and thus delete it on destruction or not. (This wrapper API is semi-transparent, but if ones reputation is dragged into the mud one becomes able to comprehend it.) Comment Actions Thank you for bringing the problems with rP19703 to our attention Krinkle! It's much appreciated. Comment Actions Sorry I was wrong - I thought the missing Session destructor fixed it, but the Jingle class also needs one. Too much nesting! It's not a leak because this is never constructed with owned = true, and doing so (and consequently deleting it if true) would cause a segfault (should be double free). For consistency it seems cleaner to me to keep m_Owned and actually use it (in the destructor).
| |||||||||||||||||||||||||