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

 
 

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

Submitted by:  Garrett Euler <ggeuler>
Submitted on:  Thu 16 Jun 2016 07:13:04 PM UTC  
 
Category: InterpreterSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Matlab Compatibility
Status: FixedAssigned to: Lachlan Andrew <lachlan>
Originator Name: Garrett EulerOpen/Closed: Closed
Release: devOperating System: Any

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)

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>
Project 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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project Administrator
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:
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:

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

This leads to strange results for mixed inputs in Octave:

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

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 20 Jun 2016 01:18:40 AM UTCrik5StatusPatch Submitted=>Fixed
      Open/ClosedOpen=>Closed
    Fri 17 Jun 2016 02:05:43 AM UTClachlanAssigned toNone=>lachlan
    Fri 17 Jun 2016 01:42:42 AM UTClachlanAttached File-=>Added bug_48247_addpath_order.cset, #37508
      StatusConfirmed=>Patch Submitted
      Originator Name=>Garrett Euler
    Thu 16 Jun 2016 08:33:06 PM UTCmtmillerStatusNone=>Confirmed
    Thu 16 Jun 2016 07:13:04 PM UTCggeulerAttached File-=>Added octave_addpath.diff, #37506

    Back to the top


    Powered by Savane 3.1-cleanup1