Re: [RFC] xfullpath and new regression test xfullpath.exp

[Joel Brobecker <brobecker at ACT-Europe dot FR>]

> Thanks.  However, this patch will not DTRT on DOS and Windows systems, 
> I'm afraid.  Details below.

I'm quite ignorant of the DOS and Windows platforms, so I'm not too
surprised, somehow. I thought that since only cygwin-gdb was supported,
I did not have to deal with DOS-like formats. Thanks for correcting me.

> This kind of detailed description should go into the source, IMHO.  It's 
> okay to have it in the ChangeLog as well, but having it _only_ in the 
> ChangeLog is not a good idea: people who read the code don't expect to 
> find the associated comments in the logs.

Right, I will put this in the code as well.

> 
> + /*
> +  * xfullpath
> +  *
> +  * Return a copy of FILENAME, with its directory prefix canonicalized
> +  * by gdb_realpath.
> +  */
> 
> I believe this is not the comment style the GNU project uses.

Is the following style more in line with the GNU project style?

  /* Return a copy of FILENAME, with its directory prefix canonicalized
     by gdb_realpath.  */

> This doesn't work correctly when the input name is something like 
> "d:foo".  lbasename returns a pointer to `f', so you end up with the 
> directory being "d:", and the rest of the code will generate "d:/foo", 
> which is something very different in semantics.  The Right Thing to do
> in this case is to return either the unmodified "d:foo" or "d:./foo".

Ah, Ok, I did not know this, I thought that d:foo and d:/foo were
equivalent. Shouldn't xfullpath return d:<cwd>/foo instead? What do you
think of the following pseudo-code?

   [...]
   dir_name = alloca ((size_t) (base_name - filename + 2));
   /* Allocate enough space to store the dir_name + plus one extra
      character sometimes needed under Windows (see below), and
      then the closing \000 character */
   strncpy (dir_name, filename, base_name - filename);
   dir_name[base_name - filename] = '\000';

   #if defined(__CYGWIN__) || defined(__MINGW32__)
   if (strlen (dir_name) == 2 &&
       is_letter (dir_name[0]) && dir_name[1] == ':')
     {
       dir_name[2] = '.';
       dir_name[3] = '\000';
     }
   #endif

   real_path = gdb_realpath (dir_name);
   [...]

> This doesn't work if the input is "\\", which is also a root directory.
> You should use the more portable IS_DIR_SEPARATOR macro (defined on 
> include/filenames.h) instead.

Cool macro.

> Finally, I don't understand why do you avoid duplicate slashes when
> dir_name is "/", but not if it is "/foo/bar/".  Why not handle the 
> other case as well?  (This issue is a general one, not something specific 
> to MS-Windows, although the case of "d:/", the root directory on drive d:, 
> was what initially brought that to my attention.)

Since I thought that only cygwin-style filenames were supported, I
thought that the the double-slash issue would only happen with the root
directory.

Is this better if I modify the last part of the function to be
(can I consider that SLASH_STRING will always be one character long?):

   if (IS_DIR_SEPARATOR (real_path[strlen (real_path) - 1]))
     result = concat (real_path, base_name, NULL);
   else
     result = concat (real_path, SLASH_STRING, base_name, NULL);

Thanks,
-- 
Joel