use weak_ptr for symbol_scope_rep and symbol_record_rep pointers (bug #53092) * symrec.h, symrec.cc (symbol_record_rep::m_fwd_scope): Manage with std::weak_ptr. Change all uses. Wrap symbol_record_rep and symbol_scope_rep pointers in std::shared_ptr<>. * symscope.h, symscope.cc (symbol_scope_rep::m_parent): Manage with std::weak_ptr. Change all uses. Wrap symbol_record_rep and symbol_scope_rep pointers in std::shared_ptr<>. (symbol_scope_rep::m_parent_fcn): Delete. (symbol_scope_rep::set_parent): Simplify. (symbol_scope_rep): Derive from std::enable_shared_from_this. (symbol_scope_rep::look_nonlocal): Use shared_from_this instead of raw pointer to this. (symbol_scope_rep::get_rep_ptr, symbol_scope_rep::parent_scope_rep, symbol_scope_rep::parent_fcn, symbol_scope::parent_fcn): Delete. Delete <, <=, >=, and > operators. * call-stack.cc (call_stack::goto_frame, call_stack::backtrace): Don't display or return pointer to symbol_scope_rep. diff --git a/libinterp/corefcn/call-stack.cc b/libinterp/corefcn/call-stack.cc --- a/libinterp/corefcn/call-stack.cc +++ b/libinterp/corefcn/call-stack.cc @@ -394,7 +394,6 @@ namespace octave << " at line " << elt.m_line << " column " << elt.m_column << " [" << elt.fcn_file_name () << "] " - << " (scope = " << elt.m_scope.get_rep_ptr () << "[context = " << elt.m_context << "])" << std::endl; } @@ -578,14 +577,12 @@ namespace octave Cell& name = retval.contents (1); Cell& line = retval.contents (2); Cell& column = retval.contents (3); - Cell& scope = retval.contents (4); Cell& context = retval.contents (5); octave_idx_type k = 0; for (const auto& frm : frames) { - scope(k) = frm.m_scope.get_rep_ptr (); context(k) = frm.m_context; file(k) = frm.fcn_file_name (); name(k) = frm.fcn_name (print_subfn); diff --git a/libinterp/corefcn/symrec.cc b/libinterp/corefcn/symrec.cc --- a/libinterp/corefcn/symrec.cc +++ b/libinterp/corefcn/symrec.cc @@ -49,7 +49,8 @@ namespace octave // only in symbol_record::symbol_record_rep::varref and // symbol_record::symbol_record_rep::varval. - return m_fwd_scope ? m_fwd_scope->current_context () : 0; + auto t_fwd_scope = m_fwd_scope.lock (); + return t_fwd_scope ? t_fwd_scope->current_context () : 0; } void @@ -64,8 +65,8 @@ namespace octave mark_persistent (); } - symbol_record::symbol_record_rep * - symbol_record::symbol_record_rep::dup (symbol_scope_rep *new_scope) const + std::shared_ptr<symbol_record::symbol_record_rep> + symbol_record::symbol_record_rep::dup (const std::shared_ptr<symbol_scope_rep>& new_scope) const { // FIXME: is this the right thing do to? if (auto t_fwd_rep = m_fwd_rep.lock ()) @@ -73,8 +74,9 @@ namespace octave static const context_id FIXME_CONTEXT = 0; - return new symbol_record_rep (m_name, varval (FIXME_CONTEXT), - m_storage_class); + return std::shared_ptr<symbol_record_rep> + (new symbol_record_rep (m_name, varval (FIXME_CONTEXT), + m_storage_class)); } octave_value diff --git a/libinterp/corefcn/symrec.h b/libinterp/corefcn/symrec.h --- a/libinterp/corefcn/symrec.h +++ b/libinterp/corefcn/symrec.h @@ -80,7 +80,7 @@ namespace octave symbol_record_rep (const std::string& nm, const octave_value& v, unsigned int sc) - : m_storage_class (sc), m_name (nm), m_fwd_scope (nullptr), + : m_storage_class (sc), m_name (nm), m_fwd_scope (), m_fwd_rep (), m_value_stack () { m_value_stack.push_back (v); @@ -478,7 +478,7 @@ namespace octave void init_persistent (void); - void bind_fwd_rep (symbol_scope_rep *fwd_scope, + void bind_fwd_rep (const std::shared_ptr<symbol_scope_rep>& fwd_scope, const std::shared_ptr<symbol_record_rep>& fwd_rep) { // If this object is already bound to another scope (for @@ -504,11 +504,12 @@ namespace octave // global in a script remain linked as globals in the // enclosing scope. - m_fwd_scope = nullptr; + m_fwd_scope = std::weak_ptr<symbol_scope_rep> (); m_fwd_rep.reset (); } - symbol_record_rep * dup (symbol_scope_rep *new_scope) const; + std::shared_ptr<symbol_record_rep> + dup (const std::shared_ptr<symbol_scope_rep>& new_scope) const; octave_value dump (context_id context) const; @@ -522,7 +523,7 @@ namespace octave std::string m_name; - symbol_scope_rep *m_fwd_scope; + std::weak_ptr<symbol_scope_rep> m_fwd_scope; std::weak_ptr<symbol_record_rep> m_fwd_rep; @@ -543,7 +544,7 @@ namespace octave ~symbol_record (void) = default; - symbol_record dup (symbol_scope_rep *sid) const + symbol_record dup (const std::shared_ptr<symbol_scope_rep>& sid) const { return symbol_record (m_rep->dup (sid)); } @@ -652,7 +653,8 @@ namespace octave unsigned int storage_class (void) const { return m_rep->storage_class (); } - void bind_fwd_rep (symbol_scope_rep *fwd_scope, const symbol_record& sr) + void bind_fwd_rep (const std::shared_ptr<symbol_scope_rep>& fwd_scope, + const symbol_record& sr) { m_rep->bind_fwd_rep (fwd_scope, sr.m_rep); } @@ -669,7 +671,9 @@ namespace octave std::shared_ptr<symbol_record_rep> m_rep; // NEW_REP must be dynamically allocated or nullptr. - symbol_record (symbol_record_rep *new_rep) : m_rep (new_rep) { } + symbol_record (const std::shared_ptr<symbol_record_rep>& new_rep) + : m_rep (new_rep) + { } octave_value find_function (const std::string& name, const octave_value_list& args) const; diff --git a/libinterp/corefcn/symscope.cc b/libinterp/corefcn/symscope.cc --- a/libinterp/corefcn/symscope.cc +++ b/libinterp/corefcn/symscope.cc @@ -93,7 +93,9 @@ namespace octave { symbol_record ret (name); - if (m_is_nested && m_parent && m_parent->look_nonlocal (name, ret)) + auto t_parent = m_parent.lock (); + + if (m_is_nested && t_parent && t_parent->look_nonlocal (name, ret)) return m_symbols[name] = ret; else { @@ -141,8 +143,10 @@ namespace octave if (p != m_subfunctions.end ()) return p->second; - if (m_parent) - return m_parent->find_subfunction (name); + auto t_parent = m_parent.lock (); + + if (t_parent) + return t_parent->find_subfunction (name); return octave_value (); } @@ -160,31 +164,17 @@ namespace octave } void - symbol_scope_rep::set_parent (symbol_scope_rep *p) + symbol_scope_rep::set_parent (const std::shared_ptr<symbol_scope_rep>& parent) { - m_parent = p; - - if (m_parent) - { - // If m_parent is the top-level scope, there will be no parent - // function. - - octave_function *current_fcn = function (); - - if (current_fcn && current_fcn->is_anonymous_function ()) - { - octave_function *parent_fcn = m_parent->function (); - - if (parent_fcn) - m_parent_fcn = octave_value (parent_fcn, true); - } - } + m_parent = std::weak_ptr<symbol_scope_rep> (parent); } void symbol_scope_rep::update_nest (void) { - if (m_parent) + auto t_parent = m_parent.lock (); + + if (t_parent) { // fix bad symbol_records for (auto& nm_sr : m_symbols) @@ -192,7 +182,7 @@ namespace octave symbol_record& ours = nm_sr.second; if (! ours.is_formal () - && m_is_nested && m_parent->look_nonlocal (nm_sr.first, ours)) + && m_is_nested && t_parent->look_nonlocal (nm_sr.first, ours)) { if (ours.is_global () || ours.is_persistent ()) error ("global and persistent may only be used in the topmost level in which a nested variable is used"); @@ -219,12 +209,14 @@ namespace octave table_iterator p = m_symbols.find (name); if (p == m_symbols.end ()) { - if (m_is_nested && m_parent) - return m_parent->look_nonlocal (name, result); + auto t_parent = m_parent.lock (); + + if (m_is_nested && t_parent) + return t_parent->look_nonlocal (name, result); } else if (! p->second.is_automatic ()) { - result.bind_fwd_rep (this, p->second); + result.bind_fwd_rep (shared_from_this (), p->second); return true; } @@ -232,7 +224,8 @@ namespace octave } void - symbol_scope_rep::bind_script_symbols (symbol_scope_rep *curr_scope) + symbol_scope_rep::bind_script_symbols + (const std::shared_ptr<symbol_scope_rep>& curr_scope) { for (auto& nm_sr : m_symbols) nm_sr.second.bind_fwd_rep (curr_scope, diff --git a/libinterp/corefcn/symscope.h b/libinterp/corefcn/symscope.h --- a/libinterp/corefcn/symscope.h +++ b/libinterp/corefcn/symscope.h @@ -50,6 +50,7 @@ namespace octave class symbol_scope; class symbol_scope_rep + : public std::enable_shared_from_this<symbol_scope_rep> { public: @@ -64,9 +65,8 @@ namespace octave subfunctions_iterator; symbol_scope_rep (const std::string& name = "") - : m_name (name), m_symbols (), m_subfunctions (), - m_fcn (nullptr), m_parent (nullptr), m_parent_fcn (), - m_children (), m_is_nested (false), + : m_name (name), m_symbols (), m_subfunctions (), m_fcn (nullptr), + m_parent (), m_children (), m_is_nested (false), m_is_static (false), m_context (0) { } @@ -91,19 +91,20 @@ namespace octave void mark_static (void) { m_is_static = true; } - symbol_scope_rep * parent_scope_rep (void) const { return m_parent; } - - octave_value parent_fcn (void) const { return m_parent_fcn; } + std::shared_ptr<symbol_scope_rep> parent_scope_rep (void) const + { + return m_parent.lock (); + } - symbol_scope_rep * dup (void) const + std::shared_ptr<symbol_scope_rep> dup (void) const { - symbol_scope_rep *new_sid = new symbol_scope_rep (m_name); + std::shared_ptr<symbol_scope_rep> new_sid + = std::shared_ptr<symbol_scope_rep> (new symbol_scope_rep (m_name)); for (const auto& nm_sr : m_symbols) new_sid->insert_symbol_record (nm_sr.second.dup (new_sid)); new_sid->m_parent = m_parent; - new_sid->m_parent_fcn = m_parent_fcn; return new_sid; } @@ -128,7 +129,8 @@ namespace octave return p->second; } - void inherit_internal (symbol_scope_rep& donor_scope_rep) + void inherit_internal + (const std::shared_ptr<symbol_scope_rep>& donor_scope_rep) { for (auto& nm_sr : m_symbols) { @@ -140,7 +142,7 @@ namespace octave if (nm != "__retval__") { - octave_value val = donor_scope_rep.varval (nm); + octave_value val = donor_scope_rep->varval (nm); if (val.is_defined ()) { @@ -153,14 +155,16 @@ namespace octave } } - void inherit (symbol_scope_rep *donor_scope_rep) + void inherit (const std::shared_ptr<symbol_scope_rep>& donor_scope_rep) { - while (donor_scope_rep) + std::shared_ptr<symbol_scope_rep> dsr = donor_scope_rep; + + while (dsr) { - inherit_internal (*donor_scope_rep); + inherit_internal (dsr); - if (donor_scope_rep->is_nested ()) - donor_scope_rep = parent_scope_rep (); + if (dsr->is_nested ()) + dsr = parent_scope_rep (); else break; } @@ -513,13 +517,13 @@ namespace octave void set_function (octave_user_function *fcn) { m_fcn = fcn; } - void set_parent (symbol_scope_rep *p); + void set_parent (const std::shared_ptr<symbol_scope_rep>& parent); void update_nest (void); bool look_nonlocal (const std::string& name, symbol_record& result); - void bind_script_symbols (symbol_scope_rep *curr_scope); + void bind_script_symbols (const std::shared_ptr<symbol_scope_rep>& curr_scope); void unbind_script_symbols (void); @@ -545,8 +549,7 @@ namespace octave octave_user_function *m_fcn; // Parent of nested function (may be null). - symbol_scope_rep *m_parent; - octave_value m_parent_fcn; + std::weak_ptr<symbol_scope_rep> m_parent; // Child nested functions. std::vector<symbol_scope> m_children; @@ -571,7 +574,9 @@ namespace octave // NEW_REP must be dynamically allocated or nullptr. If it is // nullptr, the scope is invalid. - symbol_scope (symbol_scope_rep *new_rep = nullptr) : m_rep (new_rep) { } + symbol_scope (const std::shared_ptr<symbol_scope_rep> new_rep = nullptr) + : m_rep (new_rep) + { } symbol_scope (const symbol_scope&) = default; @@ -611,15 +616,17 @@ namespace octave m_rep->mark_static (); } - symbol_scope_rep * parent_scope (void) const + std::shared_ptr<symbol_scope_rep> parent_scope (void) const { return m_rep ? m_rep->parent_scope_rep () : nullptr; } - octave_value parent_fcn (void) const + octave_value parent_fcn (void) const; +#if 0 { return m_rep ? m_rep->parent_fcn () : octave_value (); } +#endif symbol_scope dup (void) const { @@ -906,12 +913,6 @@ namespace octave m_rep->set_parent (p.get_rep ()); } - void set_parent (symbol_scope_rep *p) - { - if (m_rep) - m_rep->set_parent (p); - } - void update_nest (void) { if (m_rep) @@ -935,9 +936,9 @@ namespace octave m_rep->unbind_script_symbols (); } - symbol_scope_rep * get_rep (void) const + std::shared_ptr<symbol_scope_rep> get_rep (void) const { - return m_rep.get (); + return m_rep; } friend bool operator == (const symbol_scope& a, const symbol_scope& b) @@ -950,28 +951,6 @@ namespace octave return a.m_rep != b.m_rep; } - friend bool operator < (const symbol_scope& a, const symbol_scope& b) - { - return a.m_rep < b.m_rep; - } - - friend bool operator <= (const symbol_scope& a, const symbol_scope& b) - { - return a.m_rep <= b.m_rep; - } - - friend bool operator >= (const symbol_scope& a, const symbol_scope& b) - { - return a.m_rep >= b.m_rep; - } - - friend bool operator > (const symbol_scope& a, const symbol_scope& b) - { - return a.m_rep > b.m_rep; - } - - symbol_scope_rep * get_rep_ptr (void) const { return m_rep.get (); } - private: std::shared_ptr<symbol_scope_rep> m_rep; @@ -980,11 +959,6 @@ namespace octave { return m_rep ? m_rep->dump_symbols_map () : octave_value (); } - - symbol_scope_rep * parent_scope_rep (void) const - { - return m_rep ? m_rep->parent_scope_rep () : nullptr; - } }; }

--- a/libinterp/corefcn/symrec.h	Thu Feb 08 01:08:36 2018 -0500
+++ b/libinterp/corefcn/symrec.h	Thu Feb 08 04:06:03 2018 -0500
@@ -80,7 +80,7 @@
 
       symbol_record_rep (const std::string& nm, const octave_value& v,
                          unsigned int sc)
-        : m_storage_class (sc), m_name (nm), m_fwd_scope (nullptr),
+        : m_storage_class (sc), m_name (nm), m_fwd_scope (),
           m_fwd_rep (), m_value_stack ()
       {
         m_value_stack.push_back (v);
@@ -478,7 +478,7 @@
 
       void init_persistent (void);
 
-      void bind_fwd_rep (symbol_scope_rep *fwd_scope,
+      void bind_fwd_rep (const std::shared_ptr<symbol_scope_rep>& fwd_scope,
                          const std::shared_ptr<symbol_record_rep>& fwd_rep)
       {
         // If this object is already bound to another scope (for
@@ -504,11 +504,12 @@
         // global in a script remain linked as globals in the
         // enclosing scope.
 
-        m_fwd_scope = nullptr;
+        m_fwd_scope = std::weak_ptr<symbol_scope_rep> ();
         m_fwd_rep.reset ();
       }
 
-      symbol_record_rep * dup (symbol_scope_rep *new_scope) const;
+      std::shared_ptr<symbol_record_rep>
+      dup (const std::shared_ptr<symbol_scope_rep>& new_scope) const;
 
       octave_value dump (context_id context) const;
 
@@ -522,7 +523,7 @@
 
       std::string m_name;
 
-      symbol_scope_rep *m_fwd_scope;
+      std::weak_ptr<symbol_scope_rep> m_fwd_scope;
 
       std::weak_ptr<symbol_record_rep> m_fwd_rep;
 
@@ -543,7 +544,7 @@
 
     ~symbol_record (void) = default;
 
-    symbol_record dup (symbol_scope_rep *sid) const
+    symbol_record dup (const std::shared_ptr<symbol_scope_rep>& sid) const
     {
       return symbol_record (m_rep->dup (sid));
     }
@@ -652,7 +653,8 @@
 
     unsigned int storage_class (void) const { return m_rep->storage_class (); }
 
-    void bind_fwd_rep (symbol_scope_rep *fwd_scope, const symbol_record& sr)
+    void bind_fwd_rep (const std::shared_ptr<symbol_scope_rep>& fwd_scope,
+                       const symbol_record& sr)
     {
       m_rep->bind_fwd_rep (fwd_scope, sr.m_rep);
     }
@@ -669,7 +671,9 @@
     std::shared_ptr<symbol_record_rep> m_rep;
 
     // NEW_REP must be dynamically allocated or nullptr.
-    symbol_record (symbol_record_rep *new_rep) : m_rep (new_rep) { }
+    symbol_record (const std::shared_ptr<symbol_record_rep>& new_rep)
+      : m_rep (new_rep)
+    { }
 
     octave_value find_function (const std::string& name,
                                 const octave_value_list& args) const;