Mercurial > octave
changeset 25201:c80323fe4938 stable
improve handling of global symbols (bug #53599)
* load-save.cc (install_loaded_variable): For global values, check
global status and link to global symbol if needed.
* symrec.h (symbol_record::symbol_record_rep::unbind_global_rep):
New function.
(symbol_record::symbol_record_rep::clear): Use unbind_global_rep to
clearing global symbols.
(symbol_record::symbol_record_rep::is_marked_global): New function.
(symbol_record::symbol_record_rep::is_global): Use it.
(symbol_record::symbol_record_rep::bind_fwd_rep): Don't forward again
if symbol is already global.
(symbol_record::unbind_fwd_rep): Eliminate argument. Change all uses.
(symbol_record::symbol_record_rep::unbind_fwd_rep):
Eliminate argument. Simplify.
* test/bug-53599.tst: New file.
* test/module.mk: Update.
| author | John W. Eaton <jwe@octave.org> |
|---|---|
| date | Tue, 10 Apr 2018 22:22:37 -0400 |
| parents | b3ee0179d7b0 |
| children | d12271fe1504 6afed459d063 |
| files | libinterp/corefcn/load-save.cc libinterp/corefcn/symrec.h libinterp/corefcn/symscope.cc libinterp/corefcn/symscope.h test/bug-53599.tst test/module.mk |
| diffstat | 6 files changed, 83 insertions(+), 42 deletions(-) [+] |
line wrap: on
line diff
--- a/libinterp/corefcn/load-save.cc Tue Apr 10 08:39:24 2018 +0200 +++ b/libinterp/corefcn/load-save.cc Tue Apr 10 22:22:37 2018 -0400 @@ -153,12 +153,18 @@ if (global) { - scope.clear_variable (name); - scope.mark_global (name); - symtab.assign (name, val); + octave::symbol_record sym = scope.find_symbol (name); + + if (! sym.is_global ()) + { + octave::symbol_scope global_scope = symtab.global_scope (); + octave::symbol_record global_sym = global_scope.find_symbol (name); + + sym.bind_fwd_rep (global_scope.get_rep (), global_sym); + } } - else - scope.assign (name, val); + + scope.assign (name, val); } // Return TRUE if NAME matches one of the given globbing PATTERNS.
--- a/libinterp/corefcn/symrec.h Tue Apr 10 08:39:24 2018 +0200 +++ b/libinterp/corefcn/symrec.h Tue Apr 10 22:22:37 2018 -0400 @@ -203,7 +203,7 @@ // clear the local value. if (is_global ()) - unbind_fwd_rep (); + unbind_global_rep (); if (! (is_hidden () || is_inherited ())) { @@ -280,7 +280,7 @@ if (auto t_fwd_rep = m_fwd_rep.lock ()) return t_fwd_rep->is_global (); - return m_storage_class & global; + return is_marked_global (); } bool is_persistent (void) const @@ -363,6 +363,11 @@ m_storage_class |= global; } + bool is_marked_global (void) const + { + return m_storage_class & global; + } + void mark_persistent (void) { if (auto t_fwd_rep = m_fwd_rep.lock ()) @@ -482,51 +487,51 @@ if (auto t_fwd_rep = m_fwd_rep.lock ()) { + // If this is the symbol in the global scope, then don't + // forward again! + + if (t_fwd_rep->is_marked_global ()) + return; + t_fwd_rep->bind_fwd_rep (fwd_scope, fwd_rep); - return; } m_fwd_scope = fwd_scope; m_fwd_rep = fwd_rep; } - void unbind_fwd_rep (bool recurse = true) + void unbind_fwd_rep (void) { - // When unbinding variables in a script scope, recurse will be - // false and we will only break the immediate link. By doing - // that, we ensure that any variables that are made global in - // a script remain linked as globals in the enclosing scope. - - // When unbinding variables in a (possibly nested) function - // scope, recurse will be true and we will break the link to - // the global scope, not just the immediate link to the parent - // scope of the nested function. - - if (recurse) - { - if (auto t_fwd_rep = m_fwd_rep.lock ()) - { - // Currently, it is only possible to have at most two - // levels of indirection here, either - // - // nested function -> parent function -> global scope - // - // or - // - // script -> top level scope -> global scope - // - // so we can break the recursion here by setting the - // argument to unbind_fwd_rep to false. - - t_fwd_rep->unbind_fwd_rep (false); - return; - } - } + // When unbinding variables in a script scope, we only break + // the immediate link. By doing that, we ensure that any + // variables that are made global in a script remain linked as + // globals in the enclosing scope. m_fwd_scope = std::weak_ptr<symbol_scope_rep> (); m_fwd_rep.reset (); } + void unbind_global_rep (void) + { + // Break the link to the global symbol_record_rep. These must + // forwarded, so we don't do anything unless the forward rep + // points to something. + + if (auto t_fwd_rep = m_fwd_rep.lock ()) + { + if (t_fwd_rep->is_marked_global ()) + { + // The rep this object points to is in the global + // scope, so delete the link to it. + + m_fwd_scope = std::weak_ptr<symbol_scope_rep> (); + m_fwd_rep.reset (); + } + else + t_fwd_rep->unbind_fwd_rep (); + } + } + std::shared_ptr<symbol_record_rep> dup (const std::shared_ptr<symbol_scope_rep>& new_scope) const; @@ -657,7 +662,6 @@ void mark_formal (void) { m_rep->mark_formal (); } void mark_hidden (void) { m_rep->mark_hidden (); } void mark_inherited (void) { m_rep->mark_inherited (); } - void mark_global (void) { m_rep->mark_global (); } void mark_persistent (void) { m_rep->mark_persistent (); } void mark_added_static (void) { m_rep->mark_added_static (); } @@ -669,6 +673,9 @@ void unmark_persistent (void) { m_rep->unmark_persistent (); } void unmark_added_static (void) { m_rep->unmark_added_static (); } + bool is_marked_global (void) const { return m_rep->is_marked_global (); } + void mark_global (void) { m_rep->mark_global (); } + void init_persistent (void) { m_rep->init_persistent (); } unsigned int storage_class (void) const { return m_rep->storage_class (); } @@ -679,7 +686,7 @@ m_rep->bind_fwd_rep (fwd_scope, sr.m_rep); } - void unbind_fwd_rep (bool recurse = true) { m_rep->unbind_fwd_rep (recurse); } + void unbind_fwd_rep (void) { m_rep->unbind_fwd_rep (); } octave_value dump (context_id context) const {
--- a/libinterp/corefcn/symscope.cc Tue Apr 10 08:39:24 2018 +0200 +++ b/libinterp/corefcn/symscope.cc Tue Apr 10 22:22:37 2018 -0400 @@ -237,6 +237,6 @@ symbol_scope_rep::unbind_script_symbols (void) { for (auto& nm_sr : m_symbols) - nm_sr.second.unbind_fwd_rep (false); + nm_sr.second.unbind_fwd_rep (); } }
--- a/libinterp/corefcn/symscope.h Tue Apr 10 08:39:24 2018 +0200 +++ b/libinterp/corefcn/symscope.h Tue Apr 10 22:22:37 2018 -0400 @@ -792,6 +792,10 @@ m_rep->mark_hidden (name); } + // This function should only be called for the global + // symbol_scope, and that should only happen when it is added to + // the global symbol_scope. + void mark_global (const std::string& name) { if (m_rep)
--- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/test/bug-53599.tst Tue Apr 10 22:22:37 2018 -0400 @@ -0,0 +1,23 @@ +%!function rval = sggval (val) +%! global gval +%! if (nargin == 1) +%! gval = val; +%! else +%! rval = gval; +%! endif +%!endfunction + +%!test +%! global gval +%! assert (isempty (gval)) +%! sggval (13); +%! assert (sggval (), 13); +%! assert (gval, 13); +%! clear global gval +%! assert (sggval (), []) +%! gval = 42; +%! assert (sggval (), []); +%! clear gval +%! global gval +%! gval = 42; +%! assert (sggval (), 42);