From 801d8c4af58da784de7019de6fe8f1914a3ccd69 Mon Sep 17 00:00:00 2001 From: James Turner Date: Mon, 5 Dec 2016 12:21:04 +0000 Subject: [PATCH] Revert recursive listeners for the moment. Want to verify this is the cause of crashes inside the property code, since if it's not we have a much bigger problem. This means all listeners are recursive, with a parent-chain walk on each setValue call, as has been the case since 'forever'. --- simgear/props/PropertyBasedElement.cxx | 1 - simgear/props/PropertyBasedMgr.cxx | 2 +- simgear/props/props.cxx | 104 ++++--------------------- simgear/props/props.hxx | 43 ++++------ simgear/props/props_test.cxx | 10 ++- 5 files changed, 40 insertions(+), 120 deletions(-) diff --git a/simgear/props/PropertyBasedElement.cxx b/simgear/props/PropertyBasedElement.cxx index 2241b396..f9f7fb27 100644 --- a/simgear/props/PropertyBasedElement.cxx +++ b/simgear/props/PropertyBasedElement.cxx @@ -24,7 +24,6 @@ namespace simgear //---------------------------------------------------------------------------- PropertyBasedElement::PropertyBasedElement(SGPropertyNode* node): - SGPropertyChangeListener(true /* recursive */), _node(node) { _node->addChangeListener(this); diff --git a/simgear/props/PropertyBasedMgr.cxx b/simgear/props/PropertyBasedMgr.cxx index 7d7d6b43..c133d152 100644 --- a/simgear/props/PropertyBasedMgr.cxx +++ b/simgear/props/PropertyBasedMgr.cxx @@ -30,6 +30,7 @@ namespace simgear void PropertyBasedMgr::init() { _props->addChangeListener(this); + _props->fireCreatedRecursive(); } //---------------------------------------------------------------------------- @@ -91,7 +92,6 @@ namespace simgear PropertyBasedMgr::PropertyBasedMgr( SGPropertyNode_ptr props, const std::string& name_elements, ElementFactory element_factory ): - SGPropertyChangeListener(true /* recursive*/), _props( props ), _name_elements( name_elements ), _element_factory( element_factory ) diff --git a/simgear/props/props.cxx b/simgear/props/props.cxx index 8dc3477e..aa1baf31 100644 --- a/simgear/props/props.cxx +++ b/simgear/props/props.cxx @@ -34,6 +34,7 @@ using std::cerr; # include # include # include +# include # include "PropertyInterpolationMgr.hxx" # include "vectorPropTemplates.hxx" @@ -48,8 +49,6 @@ using std::stringstream; using namespace simgear; -#define ALTERNATE_RECURSIVE 1 - //////////////////////////////////////////////////////////////////////// // Local classes. //////////////////////////////////////////////////////////////////////// @@ -2361,34 +2360,10 @@ SGPropertyNode::addChangeListener (SGPropertyChangeListener * listener, { if (_listeners == 0) _listeners = new vector; - - auto it = std::find(_listeners->begin(), _listeners->end(), listener); - if (it != _listeners->end()) { - return; - } - - _listeners->push_back(listener); - listener->register_property(this); - -#ifdef ALTERNATE_RECURSIVE - if (listener->isRecursive()) { - for (auto child : _children) { - if (initial) { - fireChildAdded(child); - } - child->addChangeListener(listener, initial); - } - } - - if (initial) { - listener->doValueChanged(this); - } -#else - if (initial) { - listener->doValueChanged(this); - fireCreatedRecursive(false); - } -#endif + _listeners->push_back(listener); + listener->register_property(this); + if (initial) + listener->valueChanged(this); } void @@ -2396,7 +2371,6 @@ SGPropertyNode::removeChangeListener (SGPropertyChangeListener * listener) { if (_listeners == 0) return; - vector::iterator it = find(_listeners->begin(), _listeners->end(), listener); if (it != _listeners->end()) { @@ -2407,14 +2381,6 @@ SGPropertyNode::removeChangeListener (SGPropertyChangeListener * listener) _listeners = 0; delete tmp; } - -#ifdef ALTERNATE_RECURSIVE - if (listener->isRecursive()) { - for (auto child : _children) { - child->removeChangeListener(listener); - } - } -#endif } } @@ -2467,13 +2433,11 @@ SGPropertyNode::fireValueChanged (SGPropertyNode * node) { if (_listeners != 0) { for (unsigned int i = 0; i < _listeners->size(); i++) { - (*_listeners)[i]->doValueChanged(node); + (*_listeners)[i]->valueChanged(node); } } -#if !defined(ALTERNATE_RECURSIVE) if (_parent != 0) _parent->fireValueChanged(node); -#endif } void @@ -2481,19 +2445,12 @@ SGPropertyNode::fireChildAdded (SGPropertyNode * parent, SGPropertyNode * child) { if (_listeners != 0) { - for (auto listen : *_listeners) { - listen->doChildAdded(parent, child); -#if defined(ALTERNATE_RECURSIVE) - if (listen->isRecursive()) { - child->addChangeListener(listen); - } -#endif + for (unsigned int i = 0; i < _listeners->size(); i++) { + (*_listeners)[i]->childAdded(parent, child); } } -#if !defined(ALTERNATE_RECURSIVE) if (_parent != 0) _parent->fireChildAdded(parent, child); -#endif } void @@ -2501,19 +2458,12 @@ SGPropertyNode::fireChildRemoved (SGPropertyNode * parent, SGPropertyNode * child) { if (_listeners != 0) { - for (auto listen : *_listeners) { - listen->childRemoved(parent, child); -#if defined(ALTERNATE_RECURSIVE) - if (listen) { - child->removeChangeListener(listen); - } -#endif + for (unsigned int i = 0; i < _listeners->size(); i++) { + (*_listeners)[i]->childRemoved(parent, child); } } -#if !defined(ALTERNATE_RECURSIVE) if (_parent != 0) _parent->fireChildRemoved(parent, child); -#endif } //------------------------------------------------------------------------------ @@ -2533,9 +2483,9 @@ SGPropertyNode::eraseChild(simgear::PropertyList::iterator child) // Implementation of SGPropertyChangeListener. //////////////////////////////////////////////////////////////////////// -SGPropertyChangeListener::SGPropertyChangeListener(bool recursive) : - _recursive(recursive) +SGPropertyChangeListener::SGPropertyChangeListener(bool recursive) { + SG_UNUSED(recursive); // for the moment, all listeners are recursive } SGPropertyChangeListener::~SGPropertyChangeListener () @@ -2554,16 +2504,7 @@ void SGPropertyChangeListener::childAdded (SGPropertyNode * node, SGPropertyNode * child) { -} - -void SGPropertyChangeListener::doChildAdded(SGPropertyNode * parent, SGPropertyNode * child) -{ - childAdded(parent, child); -} - -void SGPropertyChangeListener::doValueChanged(SGPropertyNode * node) -{ - valueChanged(node); + // NO-OP } void @@ -2576,26 +2517,15 @@ SGPropertyChangeListener::childRemoved (SGPropertyNode * parent, void SGPropertyChangeListener::register_property (SGPropertyNode * node) { - auto it = std::lower_bound(_properties.begin(), _properties.end(), node); - if (it != _properties.end()) { - if (*it == node) { - // already exists, duplicate listent - return; - } - } - - _properties.insert(it, node); + _properties.push_back(node); } void SGPropertyChangeListener::unregister_property (SGPropertyNode * node) { - auto it = std::lower_bound(_properties.begin(), _properties.end(), node); - if (it == _properties.end()) { - // not found, warn? - return; - } - + vector::iterator it = + find(_properties.begin(), _properties.end(), node); + if (it != _properties.end()) _properties.erase(it); } diff --git a/simgear/props/props.hxx b/simgear/props/props.hxx index 99912c75..dcbd9a24 100644 --- a/simgear/props/props.hxx +++ b/simgear/props/props.hxx @@ -22,8 +22,6 @@ #include #include -#include // for fabs - #include #if PROPS_STANDALONE // taken from: boost/utility/enable_if.hpp @@ -238,12 +236,19 @@ public: */ virtual SGRaw* clone() const = 0; - }; class SGRawExtended : public SGRaw { public: + /** + * Make an SGRawValueContainer from the SGRawValue. + * + * This is a virtual function of SGRawExtended so that + * SGPropertyNode::untie doesn't need to know the type of an + * extended property. + */ + virtual SGRawExtended* makeContainer() const = 0; /** * Write value out to a stream */ @@ -252,15 +257,6 @@ public: * Read value from a stream and store it. */ virtual std::istream& readFrom(std::istream& stream) = 0; - - /** - * Make an SGRawValueContainer from the SGRawValue. - * - * This is a virtual function of SGRawExtended so that - * SGPropertyNode::untie doesn't need to know the type of an - * extended property. - */ - virtual SGRawExtended* makeContainer() const = 0; }; // Choose between different base classes based on whether the value is @@ -399,7 +395,6 @@ template<> inline const char * SGRawValue::DefaultValue() return ""; } - /** * A raw value bound to a pointer. * @@ -688,7 +683,8 @@ private: template SGRawExtended* SGRawBase::makeContainer() const { - return new SGRawValueContainer(static_cast*>(this)->getValue()); + return new SGRawValueContainer(static_cast*>(this) + ->getValue()); } template @@ -738,22 +734,14 @@ public: /// Called if \a child has been removed from its \a parent. virtual void childRemoved(SGPropertyNode * parent, SGPropertyNode * child); - bool isRecursive() const { return _recursive; } - protected: SGPropertyChangeListener(bool recursive = false); - - friend class SGPropertyNode; - - virtual void register_property (SGPropertyNode * node); - virtual void unregister_property (SGPropertyNode * node); - - void doValueChanged(SGPropertyNode * node); - void doChildAdded(SGPropertyNode * parent, SGPropertyNode * child); + friend class SGPropertyNode; + virtual void register_property (SGPropertyNode * node); + virtual void unregister_property (SGPropertyNode * node); private: - const bool _recursive = false; - std::vector _properties; + std::vector _properties; }; @@ -1155,6 +1143,7 @@ public: (state ? _attr |= attr : _attr &= ~attr); } + /** * Get all of the mode attributes for the property node. */ @@ -1773,8 +1762,6 @@ protected: static simgear::PropertyInterpolationMgr* _interpolation_mgr; private: - // friend so we can poll the SGRaw for tied properties directly. - friend class SGPropertyChangeListener; // Get the raw value bool get_bool () const; diff --git a/simgear/props/props_test.cxx b/simgear/props/props_test.cxx index 38318734..9a5a19cc 100644 --- a/simgear/props/props_test.cxx +++ b/simgear/props/props_test.cxx @@ -433,7 +433,6 @@ class TestListener : public SGPropertyChangeListener { public: TestListener(SGPropertyNode* root, bool recursive = false) : - SGPropertyChangeListener(recursive), _root(root) {} virtual void valueChanged(SGPropertyNode* node) override @@ -612,6 +611,8 @@ void testListener() defineSamplePropertyTree(tree); // ensure basic listenter is not recursive + // disabled for now, since all listeners are recurisve +#if 0 { TestListener l(tree.get(), false /* not recursive */); tree->getNode("position/body")->addChangeListener(&l); @@ -629,6 +630,7 @@ void testListener() SG_VERIFY(l.checkAdded("position/body", tree->getNode("position/body/new"))); } +#endif tree = new SGPropertyNode; defineSamplePropertyTree(tree); @@ -702,11 +704,13 @@ void testListener() // ensure additional calls to fireChildrenRecursive don't cause multiple adds - SG_VERIFY(ensureNListeners(tree->getNode("position/body"), 1)); + // disabled for now since we don't add recursive listeners to sub-trees + // SG_VERIFY(ensureNListeners(tree->getNode("position/body"), 1)); SG_VERIFY(ensureNListeners(tree->getNode("controls"), 0)); tree->getNode("position/body")->fireCreatedRecursive(); - SG_VERIFY(ensureNListeners(tree->getNode("position/body"), 1)); + // disabled for now since we don't add recursive listeners to sub-trees + //SG_VERIFY(ensureNListeners(tree->getNode("position/body"), 1)); } }