bugGNU Octave - Bugs: bug #43769, error: invalid meta.package...

 
 

bug #43769: error: invalid meta.package indexing when using strfind function

Submitter:  Rik <rik5>
Submitted:  Sat 06 Dec 2014 09:07:19 PM UTC
   
 
Category:  Interpreter Severity:  5 - Blocker
Priority:  5 - Normal Item Group:  Regression
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 08 Apr 2015 08:53:07 PM UTC, comment #15: 
John W. Eaton <jwe>
Group administrator
Wed 08 Apr 2015 08:08:31 PM UTC, comment #14: 

Rik:

The get_loader function is called many times to get the default_loader when the (package) name is empty, and also sometimes for things like foo.bar (in this case name will be "foo") and that is the only time we check whether "foo" is a package name.  This kind of thing doesn't happen frequently now so I expect the performance hit of the new (relatively expensive) is_package function will be low.  It might be good to have another way to cache the list of packages so that it is faster to determine whether a name can be a package, but I'm not sure how to properly do that when removing directories from the list.  I don't think package names are unique, are they?  I mean, +mypack could appear in multiple directories in the loadpath, correct?

John W. Eaton <jwe>
Group administrator
Mon 06 Apr 2015 12:08:58 PM UTC, comment #13: 

If there is a question about performance that should at least be a testable proposition.  I ran hg id 20030:91e1da1d1918 without (Original) and with (New) the latest patch from jwe.  I don't see much difference, but maybe classdef.tst is not a sufficient test.  Also, the OrigiIf there is a question about performance that should at least be a testable proposition.  I ran hg id 20030:91e1da1d1918 without (Original) and with (New) the latest patch from jwe.  I don't see much difference, but maybe classdef.tst is not a sufficient test.  Also, the Original code already contains the first patch that jwe committed (19641:8fe29850fb74).  Maybe the comparison should be to the code before any changes were made.


cmd1: run-octave -f --no-gui
cmd2: cd test/classdef
cmd3: tic; test ('classdef.tst'); toc

Original
--------------------------------------------------------------------------------
Elapsed time is 0.229454 seconds.
Elapsed time is 0.156741 seconds.
Elapsed time is 0.156746 seconds.
Elapsed time is 0.158385 seconds.

clear -c
Elapsed time is 0.227259 seconds.
Elapsed time is 0.159787 seconds.

clear -c
Elapsed time is 0.228508 seconds.

New
--------------------------------------------------------------------------------
Elapsed time is 0.232015 seconds.
Elapsed time is 0.158199 seconds.
Elapsed time is 0.157269 seconds.
Elapsed time is 0.157413 seconds.

clear -c
Elapsed time is 0.226182 seconds.
Elapsed time is 0.161368 seconds.
Elapsed time is 0.159671 seconds.

clear -c
Elapsed time is 0.227135 seconds.



nal code already contains the first patch that jwe committed (19641:8fe29850fb74).  Maybe the comparison should be to the code before any changes were made.


cmd1: run-octave -f --no-gui
cmd2: cd test/classdef
cmd3: tic; test ('classdef.tst'); toc

Original
--------------------------------------------------------------------------------
Elapsed time is 0.229454 seconds.
Elapsed time is 0.156741 seconds.
Elapsed time is 0.156746 seconds.
Elapsed time is 0.158385 seconds.

clear -c
Elapsed time is 0.227259 seconds.
Elapsed time is 0.159787 seconds.

clear -c
Elapsed time is 0.228508 seconds.

New
--------------------------------------------------------------------------------
Elapsed time is 0.232015 seconds.
Elapsed time is 0.158199 seconds.
Elapsed time is 0.157269 seconds.
Elapsed time is 0.157413 seconds.

clear -c
Elapsed time is 0.226182 seconds.
Elapsed time is 0.161368 seconds.
Elapsed time is 0.159671 seconds.

clear -c
Elapsed time is 0.227135 seconds.




Rik <rik5>
Group administrator
Fri 03 Apr 2015 04:58:46 PM UTC, comment #12: 

Maybe the performance hit with my latest patch is not too large because the is_package call that iterates over the list of directories in the path only hapapens if the package name is provided?  So in all the calls like


  return instance_ok ()
      ? instance->get_loader (pack_name).find_method (class_name, meth,
                                                      dir_name)
      : std::string ();


in which pack_name is an empty string, no lookup or insertion is attempted anyway.

John W. Eaton <jwe>
Group administrator
Fri 03 Apr 2015 04:54:26 PM UTC, comment #11: 

Michael, only inserting when doing add, move, or remove avoids the original problem, but then it fails for the thing you mentioned in comment #6.

John W. Eaton <jwe>
Group administrator
Fri 03 Apr 2015 04:26:19 PM UTC, comment #10: 

The trouble may come from the fact that get_loader is supposed to return a valid loader, as expected by callers. If you don't create loaders on-the-fly, what are you going to return and will the callers still behave consistently? I think that's what you have to tackle.


Michael Goffioul <goffioul>
Fri 03 Apr 2015 04:00:48 PM UTC, comment #9: 

I did think about performance, but I didn't see how to reliably cache the list of package directories.  I'll try the other approach you mentioned.  It seems like add would be the only place where we'd need to insert the loader.  If we're moving I'd expect the package to already be in the list.  If we're removing, then I'd also expect it to be in the list, and it doesn't seem there would be much point in adding it if it was not there since it is about to be removed.  But maybe I'm missing something.

John W. Eaton <jwe>
Group administrator
Fri 03 Apr 2015 01:22:21 AM UTC, comment #8: 

No there is no reason. I believe the reason I made load_path::get_loader to create loader on-demand is to support methods like load_path::add, load_path::move or load_path::remove.

I'm wondering whether your last patch may have a non-negligible performance impact, as you'll iterate over dir_info_list and a search in package_dir_map when doing symbol lookup (get_loader being used by methods like load_path::find_fcn).

Maybe another possible approach is to add a bool argument to load_path::get_loader to enable auto-insertion, set to false by default. Then set that argument to true explicitly in the calls where auto-insertion is required (mainly in load_path::add, load_path::move and load_path::remove).

Michael Goffioul <goffioul>
Thu 02 Apr 2015 09:18:00 PM UTC, comment #7: 

How about the attached changeset instead?  It seems to avoid the original problem and the one noted in comment #6.

Michael, is there any reason to enter a name in the loader map if there is no corresponding +name package directory somewhere in the load path?

(file #33519)

John W. Eaton <jwe>
Group administrator
Sat 31 Jan 2015 04:36:52 AM UTC, comment #6: 

Unfortunately, the patch also breaks lazy-loading package objects. Let's say you have a package with a function, like +pack/fun.m, and you try to execute "pack.fun()", it'll fail if the package hasn't been loaded before.

<restart octave>

pack.fun()
=> FAIL

<restart octave>

meta.package.fromName('pack')
pack.fun()
=> SUCCEED

Michael Goffioul <goffioul>
Fri 30 Jan 2015 11:42:51 PM UTC, comment #5: 

The patch fixes the original reported problem with 'edit strfind.cc'.  There is still an issue, which I could open as another bug, that I can call a C++ function with it's file name and not get an undefined error.  Maybe this has more to deal with the parser than the classdef code?


givens.cc (1,1)
error: Invalid call to givens.  Correct usage is:

 -- Built-in Function: G = givens (X, Y)
 -- Built-in Function: [C, S] = givens (X, Y)

givens.xyz (1,1)
error: Invalid call to givens.  Correct usage is:

 -- Built-in Function: G = givens (X, Y)
 -- Built-in Function: [C, S] = givens (X, Y)


It appears that the parser hits the struct indexing operator '.' and then checks to see if the token behind it is valid.  Presumably, this should be a struct or class instance variable.  But it appears to check only that it is a valid identifier which could be a variable, but could also be a function.  Octave then processes the token which is the function call givens() with no arguments.  That produces an error.  Maybe the parser needs to be more careful about what sort of argument is on the left-hand side of the struct indexing operator.

It already knows not to allow some things, like matrices and cells.


(givens (1,1)).cc
error: matrix cannot be indexed with .
{1,2}.cc
error: cell cannot be indexed with .


Maybe this is too hard to solve though.  There could be functions which take no arguments, but return a struct, which could then be indexed.


function r = x ()
  r = struct ("cc", "Hello World");
endfunction
x.cc
ans = Hello World


So, maybe I've talked myself into believing this has to stay the way it is.  One possible improvement in clarity would be to disallow bare word functions on the left side of the indexing operator.  So


x.cc  # valid when x is a class or a struct.
x ().cc  # Required parentheses when one is really trying to index the return value of a function 'x'.



Rik <rik5>
Group administrator
Fri 30 Jan 2015 08:29:28 PM UTC, comment #4: 

I think the attached patch fixes this problem.

John W. Eaton <jwe>
Group administrator
Fri 30 Jan 2015 07:49:36 PM UTC, comment #3: 

The bug seems to be related to having a function both as a file and in the symbol table.  See this sequence with the givens() functions from givens.cc.  I should get an undefined error when using givens.cc (), but instead it somehow calls the function with arguments (maybe with *self as the first parameter?).


octave:6> givens (1,1)
ans =

   0.70711   0.70711
  -0.70711   0.70711

octave:7> givens.cc (1,1)
error: Invalid call to givens.  Correct usage is:

 -- Built-in Function: G = givens (X, Y)
 -- Built-in Function: [C, S] = givens (X, Y)


octave:7> gibens (1,1)
error: 'gibens' undefined near line 1 column 1
octave:7> which givens.cc
'givens.cc' is the file /home/rik/wip/Projects_Mine/octave-dev/libinterp/corefcn/givens.cc



Rik <rik5>
Group administrator
Fri 30 Jan 2015 07:22:44 PM UTC, comment #2: 

Confirmed here as well.

@Rik, your example only works after doing "edit strfind.cc" first, getting the error originally reported, and then your strfind example fails.

Mike Miller <mtmiller>
Group Member
Fri 30 Jan 2015 06:01:29 PM UTC, comment #1: 

A simpler example is


strfind ("strfind.cc", "cc")


This is caused by the new classdef code, somehow.

Marking as a regression so that this should be fixed before the next release.

Rik <rik5>
Group administrator
Sat 06 Dec 2014 09:07:19 PM UTC, original submission:  

I get a very strange message when running a very simple command.  This is only on the development branch--the stable and gui-release trees are fine.


run-octave -f
edit strfind.cc
error: invalid meta.package indexing
error: called from:
error:   /home/rik/wip/Projects_Mine/octave-dev/scripts/strings/index.m at line 59, column 5
error:   /home/rik/wip/Projects_Mine/octave-dev/scripts/strings/rindex.m at line 50, column 5
error:   /home/rik/wip/Projects_Mine/octave-dev/scripts/miscellaneous/edit.m at line 274, column 9


The problem is somehow specifically related to using the filename 'strfind.cc' because I can use other filenames such as 'data.cc' without problem.

Rik <rik5>
Group administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #33519:  diffs.txt added by jwe (3KiB - text/plain)
file #32952:  diffs.txt added by jwe (834B - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by nul0m
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by rik5 (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 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-04-11 rik5 StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2015-04-02 jwe Attached File- Added diffs.txt, #33519
    2015-03-06 nul0m Carbon-Copy- Added -email is unavailable-
    2015-01-30 jwe Attached File- Added diffs.txt, #32952
    2015-01-30 jwe StatusConfirmed Patch Submitted
    2015-01-30 mtmiller Severity3 - Normal 5 - Blocker
        StatusNone Confirmed
    2015-01-30 rik5 Item GroupNone Regression
        Summaryerror: invalid meta.package indexing when doing \'edit strfind.cc\' error: invalid meta.package indexing when using strfind function

    Back to the top

    Powered by Savane 3.13-cf05.
    Corresponding source code