bugGNU Octave - Bugs: bug #48247, addpath('dir1','dir2','dir3')...

 
 

bug #48247: addpath('dir1','dir2','dir3') reverses input directory order

Submitter:  Garrett Euler <ggeuler>
Submitted:  Thu 16 Jun 2016 07:13:04 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  lachlan
Originator Name:  Garrett Euler 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 20 Jun 2016 01:18:40 AM UTC, comment #7: 

@Lachlan: I thought your solution was very efficient, but possibly hard to understand, and thus difficult to maintain a few years from now when our memories have all deteriorated.

I checked in a slightly different solution (http://hg.savannah.gnu.org/hgweb/octave/rev/c46e2532d421) where I copy the argument list and then call reverse() on the list if the append option is given.  This matches up well with the existing code which also has that structure.

Rik <rik5>
Group administrator
Sat 18 Jun 2016 05:46:09 AM UTC, comment #6: 

Rob, there's no need to say "sorry for commenting" -- it is always great to have people contributing!

If you're looking for stuff to do, there is a wiki page http://wiki.octave.org/Short_projects with some short project.  Alternatively, trawling the bug pages is good too.  That is how I got hooked.

Happy coding :)

Lachlan Andrew <lachlan>
Fri 17 Jun 2016 05:40:04 PM UTC, comment #5: 

Oops - patch was submitted by Lachlan Andrew looks like i missed it before, sorry for commenting, my bad.  I did come up with my own patch, not as clean.

Rob Wilkens <robwilkens>
Fri 17 Jun 2016 02:32:53 PM UTC, comment #4: 

If you leave the default '-begin' option, it prepends one at a time, which is what you are seeing.  If you change it to '-end' (last parameter to addpath) it lists them in the expected order with append, but at the end of the list.  The secret to get the desired behavior is when -begin is selected, prepend the parameters in the reverse order.

I would (and might) do this myself but i need to get out for a bit, if no one else wants to, I might attempt this later.  Warning: I'm a bored noob looking for stuff to do, but i do have a CS education circa 1996.

Rob Wilkens <robwilkens>
Fri 17 Jun 2016 01:42:42 AM UTC, comment #3: 

I can confirm Matlab's behaviour.

It is quite easy just to change the order of looping over the arguments (see the attached patch), which seems to work.

I'm not sure if the code is sufficiently quirky to make it hard to maintain.  The alternative would be to replace "i++" by "i += increment" and have a less simple ending criterion than "i < end".

(file #37508)

Lachlan Andrew <lachlan>
Thu 16 Jun 2016 09:45:14 PM UTC, comment #2: 

Yeah I completely agree.  I wanted to do the something basically equivalent to that: concatenate all the args into a single string and then create the list of directories with split_path.  That would get rid of the outer loop (over each arg).  I couldn't see a function that did the opposite of split_path to concatenate the args with the path separator and my c++ is too weak to create my own function that does it so I went with the logic duplication.

Garrett Euler <ggeuler>
Thu 16 Jun 2016 08:33:06 PM UTC, comment #1: 

I can't confirm Matlab's behavior, but I can confirm that Octave's behavior is as you describe.

I think this function would be clearer if the splitting of the function args list into a list of directories was removed from the loop adding them to the path, instead of duplicating the logic as your patch does?

Mike Miller <mtmiller>
Group Member
Thu 16 Jun 2016 07:13:04 PM UTC, original submission:  

In Matlab the addpath function always preserves the order (read from left to right) of the input directory list such that in the path they are listed as:


        dir1
        dir2
        ...
        dirN

This is the case for either prepending or appending.

In Octave (default, current and prior releases) prepending a list of directories with the command addpath('dir1','dir2','dir3') reverses the result:

>> addpath('dir1','dir2','dir3')
>> path

Octave's search path contains the following directories:

.
dir3
dir2
dir1
...


Compare this to giving the directories in a single argument which preserves the order in the Octave path:

>> addpath('dir1:dir2:dir3')
>> path

Octave's search path contains the following directories:

.
dir1
dir2
dir3
...


This leads to strange results for mixed inputs in Octave:

>> addpath('dir1','dir2:dir3:dir4','dir5')
>> path

Octave's search path contains the following directories:

.
dir5
dir2
dir3
dir4
dir1
...


When appending directories to the path (with the '-end' flag) Octave is compatible with Matlab.

I've attached an untested patch to fix this (only touches libinterp/corefcn/load-path.cc).  The patch looks rather large for what it is.  Basically I split argument handling based on if you are appending or prepending directories to the path.  If appending, the code loops over the directories proceeding from the first entry to the last while prepending goes from the last entry to the first.  This seemed the simplest approach but I've barely ever coded in c++ so I'll leave this to those more experienced.  I did not know what the commented out regex lines are so I kept those.  Since this is a Matlab Compatibility issue and not an Octave Regression I figured this best applied to the default branch.

Apologies that I don't have a system setup yet to test this - I've hit a couple known issues at the make stage and have not worked around them.

Garrett Euler <ggeuler>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #37506:  octave_addpath.diff added by ggeuler (3KiB - application/octet-stream)

 

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 robwilkens (Posted a comment)
  • -email is unavailable- added by lachlan (Updated the item)
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by ggeuler (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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-06-20 rik5 StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2016-06-17 lachlan Assigned toNone lachlan
    2016-06-17 lachlan Attached File- Added bug_48247_addpath_order.cset, #37508
        StatusConfirmed Patch Submitted
        Originator Name Garrett Euler
    2016-06-16 mtmiller StatusNone Confirmed
    2016-06-16 ggeuler Attached File- Added octave_addpath.diff, #37506

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code