Page MenuHomeWildfire Games

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

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


Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Owners Package(Owns No Changed Paths)
rP22764: Fix unused glooxwrapper variable following rP19703, refs #2305, #5294, #5550.
Trac Tickets

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

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

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

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

Test Plan


Use clang to test for unused variables.
Use valgrind to test for memory leaks.

Diff Detail

rP 0 A.D. Public Repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

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:

Okay, it can't just be removed as-is.

Apple clang fails as follows:

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:

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

635 ↗(On Diff #8947)

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:

	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.

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).
Especially becauase the removal of m_Owned would mean that the owned constructor argument would become unused.

635 ↗(On Diff #8947)

That's not an inherited class, the class is just defined in another class.

642 ↗(On Diff #8947)

Removing this line should not compile, since the line below attempts to sets m_Owned in the constructor.

Krinkle reclaimed this revision.Aug 23 2019, 1:56 PM
This revision now requires changes to proceed.Aug 23 2019, 1:56 PM
elexis commandeered this revision.Aug 23 2019, 1:57 PM
elexis edited reviewers, added: Krinkle; removed: elexis.
This revision now requires review to proceed.Aug 23 2019, 1:57 PM
elexis updated the Trac tickets for this revision.Aug 23 2019, 1:59 PM
elexis edited the test plan for this revision. (Show Details)
This revision was not accepted when it landed; it landed in state Needs Review.Aug 23 2019, 2:04 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.