From b754ddf0fbd86ef1d2e9da536bc29d50488691f2 Mon Sep 17 00:00:00 2001 From: Lee Thomason Date: Wed, 14 Jun 2017 15:02:38 -0700 Subject: [PATCH] address feedback from review --- tinyxml2.cpp | 4 ++-- tinyxml2.h | 1 + xmltest.cpp | 19 +++++++++++++++++-- 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/tinyxml2.cpp b/tinyxml2.cpp index 9855dcf..3182c5a 100755 --- a/tinyxml2.cpp +++ b/tinyxml2.cpp @@ -797,13 +797,13 @@ void XMLNode::Unlink( XMLNode* child ) if ( child->_prev ) { child->_prev->_next = child->_next; + child->_prev = 0; } if ( child->_next ) { child->_next->_prev = child->_prev; + child->_next = 0; } child->_parent = 0; - child->_next = 0; - child->_prev = 0; } diff --git a/tinyxml2.h b/tinyxml2.h index a740dc3..864c8b9 100755 --- a/tinyxml2.h +++ b/tinyxml2.h @@ -265,6 +265,7 @@ public: } void SwapRemove(int i) { + TIXMLASSERT(i >= 0); TIXMLASSERT(i < _size); _mem[i] = _mem[_size - 1]; --_size; diff --git a/xmltest.cpp b/xmltest.cpp index a580756..b86e5af 100644 --- a/xmltest.cpp +++ b/xmltest.cpp @@ -1642,8 +1642,23 @@ int main( int argc, const char ** argv ) } { - // Oh those memory leaks. - // Only way to see these is in the (Windows) allocator tracking. + // Evil memory leaks. + // If an XMLElement (etc) is allocated via NewElement() (etc.) + // and NOT added to the XMLDocument, what happens? + // + // Previously (buggy): + // The memory would be free'd when the XMLDocument is + // destructed. But the destructor wasn't called, so that + // memory allocated by the XMLElement would not be free'd. + // In practice this meant strings allocated by the XMLElement + // would leak. An edge case, but annoying. + // Now: + // The destructor is called. But the list of unlinked nodes + // has to be tracked. This has a minor performance impact + // that can become significant if you have a lot. (But why + // would you do that?) + // The only way to see this bug is in a leak tracker. This + // is compiled in by default on Windows Debug. { XMLDocument doc; doc.NewElement("LEAK 1");