changeset 24977:2b040946dc69 stable

improve handling of clear for globals, forwarded symbols (bug #53027) * symrec.h (symbol_record::symbol_record_rep::clear): Don't return early after unbinding global. Don't check for forwarded symbol here. * symrec.h (symbol_record::is_forwarded, symbol_record::symbol_record_rep::is_forwarded): New functions. * symscope.h (symbol_scope_rep::refresh): Don't delete forwarded symbols. * test/bug-53027/bug-53027.tst, test/bug-53027/module.mk, test/bug-53027/ntest53027a.m, test/bug-53027/ntest53027b.m: New test files. * test/module.mk: Include test/bug-53027/module.mk.
author John W. Eaton <jwe@octave.org>
date Thu, 22 Mar 2018 17:34:41 -0400
parents 241b80a7d8c7
children 19cf547b1453
files libinterp/corefcn/symrec.h libinterp/corefcn/symscope.h test/bug-53027/bug-53027.tst test/bug-53027/module.mk test/bug-53027/ntest53027a.m test/bug-53027/ntest53027b.m test/module.mk
diffstat 7 files changed, 86 insertions(+), 19 deletions(-) [+]
line wrap: on
line diff
--- a/libinterp/corefcn/symrec.h	Wed Mar 21 21:53:40 2018 -0500
+++ b/libinterp/corefcn/symrec.h	Thu Mar 22 17:34:41 2018 -0400
@@ -199,31 +199,19 @@
 
       void clear (context_id context)
       {
-        // There is no need to do anything with a fowarded
-        // symbol_record_rep here.
-        //
-        // For scripts, we are never executing in the script "scope".
-        //
-        // For globals, we are only interested in breaking the link to
-        // the global value and clearing the local value, not the
-        // global one.
-
-        // For persistent values, we clear the value then unmark so
-        // that we clear the first element of the value stack.
+        // For globals, break the link to the global value first, then
+        // clear the local value.
 
         if (is_global ())
-          {
-            unbind_fwd_rep ();
-            return;
-          }
-
-        if (auto t_fwd_rep = m_fwd_rep.lock ())
-          return;
+          unbind_fwd_rep ();
 
         if (! (is_hidden () || is_inherited ()))
           {
             assign (octave_value (), context);
 
+            // For persistent values, we clear the value then unmark so
+            // that we clear the first element of the value stack.
+
             if (is_persistent ())
               unmark_persistent ();
           }
@@ -282,6 +270,11 @@
         return m_storage_class & inherited;
       }
 
+      bool is_forwarded (void) const
+      {
+        return ! m_fwd_rep.expired ();
+      }
+
       bool is_global (void) const
       {
         if (auto t_fwd_rep = m_fwd_rep.lock ())
@@ -499,6 +492,12 @@
 
       void unbind_fwd_rep (void)
       {
+        if (auto t_fwd_rep = m_fwd_rep.lock ())
+          {
+            t_fwd_rep->unbind_fwd_rep ();
+            return;
+          }
+
         // When unbinding an object, 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
@@ -629,6 +628,7 @@
     bool is_global (void) const { return m_rep->is_global (); }
     bool is_hidden (void) const { return m_rep->is_hidden (); }
     bool is_inherited (void) const { return m_rep->is_inherited (); }
+    bool is_forwarded (void) const { return m_rep->is_forwarded (); }
     bool is_persistent (void) const { return m_rep->is_persistent (); }
     bool is_added_static (void) const { return m_rep->is_added_static (); }
 
--- a/libinterp/corefcn/symscope.h	Wed Mar 21 21:53:40 2018 -0500
+++ b/libinterp/corefcn/symscope.h	Thu Mar 22 17:34:41 2018 -0400
@@ -277,7 +277,7 @@
         {
           symbol_record& sr = nm_sr.second;
 
-          if (! sr.is_persistent ())
+          if (! (sr.is_persistent () || sr.is_forwarded ()))
             sr.clear (m_context);
         }
     }
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/bug-53027/bug-53027.tst	Thu Mar 22 17:34:41 2018 -0400
@@ -0,0 +1,34 @@
+%!function load53027 (fname)
+%!  global X
+%!  X = 2;
+%!  load (fname);
+%!  assert (X, 1);
+%!endfunction
+
+%!function save53027 (fname)
+%!  global X
+%!  X = 1;
+%!  save (fname, "X");
+%!endfunction
+
+%!test <*53027>
+%! global X
+%! X = 0;
+%! fname = tmpnam ();
+%! save53027 (fname);
+%! assert (X, 1);
+%! load53027 (fname);
+%! assert (X, 1);
+%! load53027 (fname);
+%! assert (X, 1);
+%! clear X
+%! assert (exist ("X"), 0);
+
+%!test <*53027>
+%! [a, b] = ntest53027a ();
+%! assert ([a, b], [0, 0])
+
+## Not fixed yet.
+%!test <53027>
+%! [a, b] = ntest53027b ();
+%! assert ([a, b], [0, 0])
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/bug-53027/module.mk	Thu Mar 22 17:34:41 2018 -0400
@@ -0,0 +1,6 @@
+bug_53027_TEST_FILES = \
+  %reldir%/bug-53027.tst \
+  %reldir%/ntest53027a.m \
+  %reldir%/ntest53027b.m
+
+TEST_FILES += $(bug_53027_TEST_FILES)
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/bug-53027/ntest53027a.m	Thu Mar 22 17:34:41 2018 -0400
@@ -0,0 +1,11 @@
+function [m_exist, n_exist] = ntest53027a ()
+  global x
+  x = 3;
+  n_exist = nest ();
+  m_exist = exist ("x", 'var');
+  function n_exist = nest ()
+    x = 1;
+    clear x
+    n_exist = exist ("x", "var");
+  endfunction
+endfunction
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/bug-53027/ntest53027b.m	Thu Mar 22 17:34:41 2018 -0400
@@ -0,0 +1,15 @@
+function [m_exist, n_exist] = ntest53027b ()
+  global x
+  x = 3;
+  n_exist = nest ();
+  m_exist = exist ("x", 'var');
+  function n_exist = nest ()
+    ## The clear statement should operate on the variable in the
+    ## parent scope even though there is no explicit varabiable
+    ## reference in teh code (the clear function just sees a string
+    ## containing the name of the variable and the parser (correctly)
+    ## does not treat this as a special case.
+    clear x
+    n_exist = exist ("x", "var");
+  endfunction
+endfunction
--- a/test/module.mk	Wed Mar 21 21:53:40 2018 -0500
+++ b/test/module.mk	Thu Mar 22 17:34:41 2018 -0400
@@ -65,6 +65,7 @@
 include %reldir%/bug-51599/module.mk
 include %reldir%/bug-52075/module.mk
 include %reldir%/bug-52722/module.mk
+include %reldir%/bug-53027/module.mk
 include %reldir%/class-concat/module.mk
 include %reldir%/classdef/module.mk
 include %reldir%/classdef-multiple-inheritance/module.mk