Page MenuHomeWildfire Games

glooxwrapper: Remove unused private 'm_Owned' in Jingle::Session::Jingle
AbandonedPublic

Authored by Krinkle on Jul 17 2019, 2:33 AM.

Details

Reviewers
elexis
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Owners Package(Owns No Changed Paths)
Summary

Fix compiler warning (Apple LLVM version 10.0.1; clang-1001.0.46.4):

In file included from …/source/lobby/glooxwrapper/glooxwrapper.cpp:20:
…/source/lobby/glooxwrapper/glooxwrapper.h:642:10:

warning: private field 'm_Owned' is not used [-Wunused-private-field]

It appears to have been unused since its introduction in rP19703.

Test Plan

Jenkins.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
master
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8412
Build 13746: Vulcan BuildJenkins
Build 13745: arc lint + arc unit

Event Timeline

Krinkle created this revision.Jul 17 2019, 2:33 AM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/110/display/redirect

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) {}

                                                                                             ^~~~~~~
source/lobby/glooxwrapper/glooxwrapper.h
635

This should declare it I think, which from a quick glance at a Cpp help page, I believe is meant to be visible to the public: block within the same or a derived class.

Krinkle planned changes to this revision.Jul 17 2019, 7:06 PM

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.

elexis requested changes to this revision.Jul 18 2019, 12:25 AM
elexis added a subscriber: elexis.

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.

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.)

elexis abandoned this revision.Jul 19 2019, 2:26 PM

(Please pick this up again if you want to work on this, otherwise I'll assume you'll be okay with Josh and me messing with this in D2093 and D2094)

Thank you for bringing the problems with rP19703 to our attention Krinkle! It's much appreciated.