Nested functions patch

[John W. Eaton]

On  8-Mar-2012, Max Brister wrote:

| Attached is a patch to add support for nested functions along with a
| few simple test cases (in test/nest). I tired to keep modifications to
| the existing code to a minimum. Basically, I added a new storage class
| to symbol_record, nonlocal, to handle variable lookup in nested
| functions. My computer also passes the same number of tests with and
| without the patch, so I don't think I added too many horrible bugs. I
| would appreciate any feedback.

I tried your patch.  It looks like you have done a good job of
understanding what needs to be done fairly quickly.  The changes do
work in some cases, but I noticed the following problems.

For

  function f ()
    x = 1
    g (x)
    x
    function g (x)
      x = 2
    end
  end
  f ()

I see the following:

  x =  1
  x =  2
  error: `x' undefined near line 4 column 3
  error: called from:
  error:   f at line 4, column 3

I think X should still be defined to be 1 after the call to G.

Also, after executing the above code, I see a segfault when Octave
exits.

I have not done more complete testing so I don't know whether there
are other issues like this.

I noticed a few other style issues.

First, please use spaces, not TABs.

To match the style used in Octave .cc files, please write

  void
  symbol_table::install_nestfunction (const std::string& name,
                                      const octave_value& fcn,
                                      scope_id parent_scope)

Instead of

| +void symbol_table::install_nestfunction (const std::string& name,
| +                                         const octave_value& fcn,
| +                                         scope_id parent_scope)


To match the format of error messages in Octave, please don't
capitalize or use periods, so write

  ::error ("closures are not yet supported");

instead of

  ::error ("Closures are not yet supported.");

In most of Octave, we try to avoid multiple levels of dereferencing,
so instead of

| +    return record.varref (tbl->curr_fcn->active_context ());

I would write

  return record.varref (tbl->active_context ());

and define a method to do this action for tbl.  Similarly, please undo
the following change:

| @@ -2063,16 +2112,11 @@
|      return retval;
|    }
|  
| -  void insert_symbol_record (const symbol_record& sr)
| -  {
| -    table[sr.name ()] = sr;
| -  }
| -
|    void
|    do_dup_scope (symbol_table& new_symbol_table) const
|    {
|      for (table_const_iterator p = table.begin (); p != table.end (); p++)
| -      new_symbol_table.insert_symbol_record (p->second.dup ());
| +     new_symbol_table.table[p->first] = p->second.dup ();
|    }
|  
|    symbol_record do_find_symbol (const std::string& name)


I don't understand the following:

| @@ -1961,6 +2004,12 @@
|    // Map from symbol names to symbol info.
|    std::map<std::string, symbol_record> table;
|  
| +  // If we are nested, this is our parent function
| +  symbol_table *nest_parent;
| +
| +  // And these are all of our nested child functions
| +  std::vector<symbol_table*> nest_children;
| +

A symbol table is not a function.  Do you mean that nest_parent is the
symbol table for the parent function?  If so, I'm not sure that makes
sense.  There should be one symbol table with multiple scopes, one for
each function.  So it seems to me that you would want to track scopes
rather than symbol table objects.

Thinking about this more, when we have nested functions, don't the
children effectively have the same scope as the parent, except that
variables passed in the argument list or as globals are handled
separately?  So would it be possible to handle nested functions by
merging the scopes of the parents and children?  I suppose we would
have to be sure that the parents do not have access to the symbols
that appear only in the child functions.  But if the child and parent
shared the same scope, then it seems that it would simplify variable
lookup in the child.

What is the intent of the update_nest function?

| +  void do_update_nest (const std::set<std::string>& check = 
std::set<std::string> ())
| +  {
| +    if (check.size ())
| +      {
| +      for (table_iterator iter = table.begin (); iter != table.end (); 
++iter)
| +     {
| +       if (check.count (iter->first))
| +         iter->second.mark_nonlocal ();
| +       else
| +         iter->second.unmark_nonlocal ();
| +     }
| +      }
| +
| +    if (nest_children.size ())
| +      {
| +     std::set<std::string> next = check;
| +     for (table_iterator iter = table.begin (); iter != table.end (); ++iter)
| +       {
| +         if (iter->second.is_local ())
| +           next.insert (iter->first);
| +       }
| +
| +     for (std::vector<symbol_table*>::iterator iter = nest_children.begin ();
| +          iter != nest_children.end (); ++iter)
| +       (*iter)->do_update_nest (next);
| +      }
| +  }
|  };

Please fix the test files to end with newline characters.

| diff -r c1f9b54350f9 -r fbe54c9e8eb7 test/nest/recursive_nest.m
| ...
| \ No newline at end of file

Tests should not print anything.

Instead of

| +%!test
| +%! a = recursive_nest;
| +%! assert (a, 25);

you can write

  %!assert (recursive_nest (), 25)

(note that in the above, "%!assert" is handled specially by the
testing framework and there must not be a space between the "!" and
the "a".

jwe