bugGNU Octave - Bugs: bug #47705, function...

 
 

bug #47705: function "localfunctions" not implemented

Submitter:  Guillaume <gyom>
Submitted:  Fri 15 Apr 2016 11:15:11 AM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Feature Request
Status:  Fixed Assigned to:  lachlan
Originator Name:  Guillaume Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 23 May 2016 04:52:15 AM UTC, comment #18: 

I pushed the cset here (http://hg.savannah.gnu.org/hgweb/octave/rev/23a610a3f001) after adding a Compatibility Note about the problem with nested functions.

Rik <rik5>
Group administrator
Sun 22 May 2016 10:21:20 AM UTC, comment #17: 

Rik, thanks for the debugging.  I must just never have tested this case.

It seems the current localfunctions() behaviour is the best that is possible until it is possible to take handles of nested functions (see bug #39257).  Taking @c gives an error, and so it is consistent that localfunctions() cannot return a handle to c.  I'm setting the status back to "Patch submitted".

I haven't been able to find out exactly why it is not allowed to take handles of the outermost nested function -- the inner ones obviously have problems because they share memory with the outer function and so assume its stack frame exists, but that isn't a problem for the outermost since calling it would create the stack frame.

However, the code in symtab.cc (around line 1652) explicitly says


  if (nest_parent || nest_children.size ())
    curr_fcn->mark_as_nested_function ();


and the only time  is_nested_function ()  was used was the decision of whether or not to allow handles to be taken, so it wasn't an unintentional side-effect.

My cavalier side would just allow taking handles of the outermost function, and wait for someone to report breakage :-) but of course I wouldn't suggest that on anything that could possibly make its way into production code.

Lachlan Andrew <lachlan>
Sun 22 May 2016 05:17:46 AM UTC, comment #16: 

Odd.  I'm sure I got some output previously.  I'll see if the behaviour of is_nested_function has changed, or if I'm just imagining things.

Lachlan Andrew <lachlan>
Sat 21 May 2016 03:10:48 PM UTC, comment #15: 

Using gdb, I stepped through C++ code and the problem is that the conditions necessary to assign to retval never become true.


  size_t i = 0;
  for (std::list<std::string>::const_iterator p = names.begin ();
       p != names.end (); p++)
    {
      std::map<std::string, octave_value>::const_iterator q = h.find (*p);
      if (q != h.end () &&
          ! q->second.user_function_value ()->is_nested_function ())
        retval(i++) = octave_value (new octave_fcn_handle (q->second, *p));
    }


The issue is that even for subfunction 'c', is_nested_function() is returning true.

Rik <rik5>
Group administrator
Sat 21 May 2016 02:56:47 PM UTC, comment #14: 

I tried to test this but I always get an empty cell array.

The test function I used is shown below.


function tst_lfun

  list = localfunctions ()
  function b
    x = 1;
  end
  y = 1;

end

function c
  z = 1;

  function d
    w = 2;
  end

end


I also tried using the debugger and stepping through line 3 but the value of list is always empty.  Something is happening, because the size of the empty cell array is 0x1 rather than 0x0.

Rik <rik5>
Group administrator
Sat 21 May 2016 05:02:13 AM UTC, comment #13: 

Here is an amended cset.  I just put a single line into NEWS and func.txi -- should I have written any explanatory text?

(file #37222)

Lachlan Andrew <lachlan>
Fri 20 May 2016 04:47:34 PM UTC, comment #12: 

I agree with Mike.  This cset can be committed as soon as it is complete with a reference in the documentation and a mention in the NEWS file.

Rik <rik5>
Group administrator
Mon 16 May 2016 07:34:48 PM UTC, comment #11: 

This looks like its been tested and about ready to push. Would you mind amending the changeset to include adding the function to NEWS and putting a reference to the DOCSTRING in one of the doc/interpreter/*.txi files, maybe func.txi?

Mike Miller <mtmiller>
Group Member
Wed 20 Apr 2016 04:08:09 PM UTC, comment #10: 

I've started implementing a unit testing framework compatible with the one from MATLAB and, thanks to localfunctions(), I've made good progress. One missing feature from Octave is import() when dealing with +packages (I couldn't find a bug report requesting this feature). Would there be interest to have such a framework available in Octave or would it be redundant with the one already available (%!test)?
From what I can see, your patches are ready to be submitted. Many thanks, Lachlan!

Guillaume <gyom>
Mon 18 Apr 2016 11:57:33 AM UTC, comment #9: 

I can also confirm it works fine. Many thanks!

I'll try to write an implementation of functiontests() (and other related testing framework functions) at some point

Guillaume <gyom>
Sun 17 Apr 2016 12:26:26 PM UTC, comment #8: 

AFAICS localfunctions() (the latest version) works fine.

I haven't looked at the unimplemented list patch yet.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 16 Apr 2016 10:33:32 AM UTC, comment #7: 

Thanks for your help with this, Guillaume.

I've attached a patch that always returns a cell array, and includes a basic self-test.

I'm attaching a separate cset to add functiontests to the unimplemented list.  It looks a bit too hard for me to implement, though.

(file #36947, file #36948)

Lachlan Andrew <lachlan>
Fri 15 Apr 2016 05:14:55 PM UTC, comment #6: 

Many thanks, Lachlan!

It seems to work as expected. I would remove the nargout conditionals and always return a cell array (for compatibility with MATLAB) - ie no text output.

Guillaume <gyom>
Fri 15 Apr 2016 03:30:49 PM UTC, comment #5: 

Guillame, can you take a look at the attached function and let me know how it goes?  It doesn't handle nested functions (yet), but I think it should work for normal sub-functions.



(file #36942)

Lachlan Andrew <lachlan>
Fri 15 Apr 2016 02:29:02 PM UTC, comment #4: 

It just displays the output cell array as 'ans'; I guess it means that it should not be listed in the help text then.

I also notice that functiontests() does not appear in the list of unimplemented functions in Octave:

http://uk.mathworks.com/help/matlab/ref/functiontests.html

Guillaume <gyom>
Fri 15 Apr 2016 12:59:44 PM UTC, comment #3: 

That looks great!

In the first invocation, with no output variable, does it print a list of function names, one per line?  If not, can you tell me what it does?

Thanks :)

Lachlan Andrew <lachlan>
Fri 15 Apr 2016 12:15:15 PM UTC, comment #2: 

Thanks, Lachlan. Here is an attempt at writing the help text:


## -*- texinfo -*-
## @deftypefn  {} {} localfunctions ()
## @deftypefnx {} {@var{local_functions} =} localfunctions ()
## Return a list of all local functions accessible from a function.
##
## Return value @var{local_functions} is a cell array of function handles of all
## local functions accessible from the function from which @code{localfunctions}
## is called.
##
## If called from the command line, an anonymous function, or a script,
## @var{local_functions} is an empty cell array.
##
## @seealso{functiontests}
## @end deftypefn


Guillaume <gyom>
Fri 15 Apr 2016 11:43:35 AM UTC, comment #1: 

It should be straightforward.  There is already a "subfunction_names ()" method of class octave_user_code.

Writing the documentation (without violating Matlab copyright) will be half of the work.  Could you write the help text?  It would be ideal if you could put in the texinfo markup, but just the plain text would still be a big help.

Lachlan Andrew <lachlan>
Fri 15 Apr 2016 11:15:11 AM UTC, original submission:  

The function localfunctions() is not yet implemented in Octave:

http://www.mathworks.com/help/matlab/ref/localfunctions.html

I don't think there is a way to implement this as M-code. How difficult would it be to retrieve the list of local functions from the parsing of a file?

This would contribute to build a library compatible with MATLAB's testing framework:

http://www.mathworks.com/help/matlab/matlab-unit-test-framework.html

Guillaume <gyom>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by philipnienhuis
  • -email is unavailable- added by lachlan (Posted a comment)
  • -email is unavailable- added by gyom (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-05-23 rik5 StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2016-05-22 lachlan StatusIn Progress Patch Submitted
    2016-05-21 rik5 StatusReady For Test In Progress
    2016-05-21 lachlan Attached File- Added bug_47705_localfunctions_v3.cset, #37222
    2016-04-17 philipnienhuis Carbon-Copy- Added philipnienhuis
    2016-04-17 philipnienhuis StatusPatch Submitted Ready For Test
    2016-04-16 lachlan Attached File- Added bug_47705_localfunctions_v2.cset, #36947
        Attached File- Added functiontests_unimplemnted.cset, #36948
        StatusNone Patch Submitted
        Assigned toNone lachlan
    2016-04-15 lachlan Attached File- Added bug_47705_localfunctions.cset, #36942

    Back to the top

    Powered by Savane 3.13-4b4b.
    Corresponding source code