Re: difference between path / pathdef

[Ben Abbott]

On Jan 15, 2008, at 7:32 PM, John W. Eaton wrote:

> On 15-Jan-2008, Ben Abbott wrote:
>
> |
> | On Jan 15, 2008, at 6:38 PM, John W. Eaton wrote:
> |
> | > On 15-Jan-2008, Ben Abbott wrote:
> | >
> | > | Shall I run the command above prior to creating the patch?
> | >
> | > It doesn't matter since we don't care about generated files.  We  
> only
> | > need patches for the Makefile.in files that you have to modify,  
> not
> | > the generated Makefiles.
> | >
> | > jwe
> |
> | ok here's the patch, ChangeLog, and new-files. The new files are
> | placed in a sub-directories corresponding to where they belong in  
> the
> | scripts directory.
>
> OK.  It would be easier if you use text/plain for these attachments.

In the past I had problems with EOL terminations (mac vs unix vs dos)  
when I sent you patches. Thus, I've became weary of relying on my  
email to preserve the proper EOL characters. However, since I had not  
reached a conclusion of where things went bad, I'm happy to give it  
another try.

> | -## @code{savepath} returns 0.
> | +## @file{savepath} returns 0.

ok

> I think @code is better here, because in this context savepath is a
> function name, not a file.
>
> | +  elseif savepath(1) == '~'
> | +    savepath = tilde_expand (savepath);
>
> Calling tilde_expand on a filename that doesn't start with ~ is a
> no-op, so it's OK to just write
>
>  else
>    savepath = tilde_expand (savepath);
>
> But is it even necessary to explicitly call tilde_expand?  I see the
> file name is passed to exist and fopen, both of which should do tilde
> expansion on their own.  Is there some other place in your code where
> tilde expansion is needed but is not performed automatically?

To be honest, I hadn't tested it with the "~" in place. I'd just  
assumed it wouldn't work.

> | +  elseif nargin == 0
> | +    warning ("savepath: current path saved to %s",savefile)
>
> To be consistent with other code in Octave, please write
>
>  elseif (nargin == 0)

ok

> | +{
> | +  bool reinitializing_path = true;
> | +  octave_value retval;
> | +
> | +  load_path::initialize (reinitializing_path);
> | +  retval = load_path::system_path ();
> | +
> | +  return retval;
> | +}
>
> It would be OK to simply write
>
>  load_path::initialize (true);
>
>  return load_path::system_path ();

To be totally honest, I have no experience with c/c++. I consider  
myself quite fortunate that this part worked at all ;-)

> In the new pathdef.m file:
>
> |   ## Use Octave's orignal path as the default default.
> |   val = __pathorig__;
>
> In most cases (nargin and nargout are notable exceptions) we generally
> prefer
>
>  val = __pathorig__ ();

ok

> so that it is clear that this is a function call.
>
> |   ## Locate the site octaverc file (is there a better way?).
> |   prestr = [OCTAVE_HOME, filesep];
> |   poststr = [filesep, version, filesep, 'm', filesep', 'startup'];
> |   ncolon = [0, strfind(presentpath,':'), numel(presentpath)+1];
> |   site_octaverc = '';
> |   for nc = 1:(numel(ncolon)-1)
> |     pathdir = presentpath((ncolon(nc)+1):(ncolon(nc+1)-1));
> |     npre = strfind (pathdir, prestr);
> |     npost = strfind (pathdir, poststr);
> |     if (npre == 1 &&
> |         npost > numel (prestr) &&
> |         npost == numel (pathdir) - numel (poststr)+1)
> |       site_octaverc = [pathdir, filesep, 'octaverc'];
> |       break;
> |     endif
> |   endfor
>
> Yes, I think the better way is to use the value returned by
> octave_config_info ("localstartupfiledir").
>
> |   if isempty (site_octaverc) || ~exist (site_octaverc, 'file')
> |     regexp_octaverc = [prestr, '*', poststr, filesep, 'octaverc'];
> |     warning (['pathdef: could not locate `',regexp_octaverc,'''.'])
> |   endif
>
> To be consistent with the rest of Octave, please use ! instead of ~.

hmmm, time for me to make a list of preferred coding conventions for  
Octave.

> |   ## A path definition in the user octaverc has precedence over  
> the site
> |   if numel (user_pathscript)
> |     try
> |       eval (user_pathscript);
> |       val = path;
> |     catch
> |       warning (['Path defined in ',user_octaverc,' produced an  
> error'])
> |     end_try_catch
>
> I think we should try to avoid the eval here.  What is the format of
> user_pathscript?

The same as is produced by savepath.m

You can take a look by doing the following

octave:22> savepath
octave:23> user_octaverc = "~/.octaverc";
octave:24> user_pathscript = __extractpath__ (user_octaverc)
user_pathscript =

{
  [1,1] = ## Begin savepath auto-created section, do not edit
  [1,2] =   path ('.:/sw/share/octave/site/m:/sw/share/octave/site/m/ 
startup:/sw/lib/octave/3.0.0+/oct/i386-apple-darwin9.1.0:/sw/share/ 
octave/3.0.0+/m:/sw/share/octave/3.0.0+/m/audio:/sw/share/octave/ 
3.0.0+/m/control:/sw/share/octave/3.0.0+/m/control/base:/sw/share/ 
octave/3.0.0+/m/control/hinf:/sw/share/octave/3.0.0+/m/control/ 
obsolete:/sw/share/octave/3.0.0+/m/control/system:/sw/share/octave/ 
3.0.0+/m/control/util:/sw/share/octave/3.0.0+/m/deprecated:/sw/share/ 
octave/3.0.0+/m/elfun:/sw/share/octave/3.0.0+/m/finance:/sw/share/ 
octave/3.0.0+/m/general:/sw/share/octave/3.0.0+/m/geometry:/sw/share/ 
octave/3.0.0+/m/image:/sw/share/octave/3.0.0+/m/io:/sw/share/octave/ 
3.0.0+/m/linear-algebra:/sw/share/octave/3.0.0+/m/miscellaneous:/sw/ 
share/octave/3.0.0+/m/optimization:/sw/share/octave/3.0.0+/m/path:/sw/ 
share/octave/3.0.0+/m/pkg:/sw/share/octave/3.0.0+/m/plot:/sw/share/ 
octave/3.0.0+/m/polynomial:/sw/share/octave/3.0.0+/m/quaternion:/sw/ 
share/octave/3.0.0+/m/set:/sw/share/octave/3.0.0+/m/signal:/sw/share/ 
octave/3.0.0+/m/sparse:/sw/share/octave/3.0.0+/m/specfun:/sw/share/ 
octave/3.0.0+/m/special-matrix:/sw/share/octave/3.0.0+/m/startup:/sw/ 
share/octave/3.0.0+/m/statistics:/sw/share/octave/3.0.0+/m/statistics/ 
base:/sw/share/octave/3.0.0+/m/statistics/distributions:/sw/share/ 
octave/3.0.0+/m/statistics/models:/sw/share/octave/3.0.0+/m/statistics/ 
tests:/sw/share/octave/3.0.0+/m/strings:/sw/share/octave/3.0.0+/m/ 
testfun:/sw/share/octave/3.0.0+/m/time');
  [1,3] = ## End savepath auto-created section
}

octave:25>

> Also, please write
>
>  warning ("pathdef: path defined in `%s' produced an error",  
> user_octaverc);
>
> instead of using [] to concatenate.  We also prefer to prefix warnings
> and errors with the name of the function.
>
> Also in this and the other new files, please use " instead of ' to
> quote character strings and fix the formatting of if conditions to
> match the style of the rest of Octave.

ok, I've added it to my new list of coding conventions.

> Thanks,
>
> jwe


Shall I make the changes you've asked and create another patch, or  
have you already taken care of that?

Ben