Re: difference between path / pathdef

[John W. Eaton]

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.

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

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?

| +  elseif nargin == 0
| +    warning ("savepath: current path saved to %s",savefile)

To be consistent with other code in Octave, please write

  elseif (nargin == 0)

| +{
| +  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 ();

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__ ();

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

|   ## 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?

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.

Thanks,

jwe