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

 
 

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

Submitted by:  Rik <rik5>
Submitted on:  Sat 06 Dec 2014 09:07:19 PM UTC  
 
Category: InterpreterSeverity: 5 - Blocker
Priority: 5 - NormalItem Group: Regression
Status: FixedAssigned to: None
Originator Name: Open/Closed: Closed
Release: devOperating System: GNU/Linux

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Wed 08 Apr 2015 08:53:07 PM UTC, comment #15:

I pushed my change here:

http://hg.savannah.gnu.org/hgweb/octave/rev/8b501a0db1e9

John W. Eaton <jwe>
Project 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>
Project 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.

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.

Rik <rik5>
Project 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

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

John W. Eaton <jwe>
Project 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>
Project 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>
Project Member
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>
Project 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>
Project Member
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>
Project 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>
Project Member
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?

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.

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.

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

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

I think the attached patch fixes this problem.

John W. Eaton <jwe>
Project 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?).

Rik <rik5>
Project 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>
Project Administrator
Fri 30 Jan 2015 06:01:29 PM UTC, comment #1:

A simpler example is

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>
Project 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.

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>
Project Administrator

 

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

Attach File(s):
   
   
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
  • -unavailable- added by nul0m
  • -unavailable- added by goffioul (Posted a comment)
  • -unavailable- added by jwe (Posted a comment)
  • -unavailable- added by mtmiller (Posted a comment)
  • -unavailable- added by rik5 (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 10 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sat 11 Apr 2015 05:27:28 AM UTCrik5StatusPatch Submitted=>Fixed
      Open/ClosedOpen=>Closed
    Thu 02 Apr 2015 09:18:00 PM UTCjweAttached File-=>Added diffs.txt, #33519
    Fri 06 Mar 2015 03:45:13 PM UTCnul0mCarbon-Copy-=>Added -unavailable-
    Fri 30 Jan 2015 08:29:45 PM UTCjweAttached File-=>Added diffs.txt, #32952
    Fri 30 Jan 2015 08:29:28 PM UTCjweStatusConfirmed=>Patch Submitted
    Fri 30 Jan 2015 07:22:44 PM UTCmtmillerSeverity3 - Normal=>5 - Blocker
      StatusNone=>Confirmed
    Fri 30 Jan 2015 06:01:29 PM UTCrik5Item 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.1-cleanup1