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

[Eli Zaretskii <eliz@is.elta.co.il>]

On Mon, 1 Apr 2002, Joel Brobecker wrote:

> I implemented the new xfullpath function based on Andrew's comments, and
> made some adjustements to fix the problem reported in
> 
>     http://sources.redhat.com/ml/gdb-patches/2002-03/msg00345.html
> 
> while being careful not to break Tom's setup. The patch is attached.

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

>         * source.c (openp): Use xfullpath in place of gdb_realpath to
>         avoid resolving the basename part of filenames when the
>         associated file is a symbolic link. This fixes a potential
>         inconsistency between the filenames known to GDB and the
>         filenames it prints in the annotations.

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.

+ /*
+  * 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.

Here are specific comments about the code part of your patch:

+ char *
+ xfullpath (const char *filename)
+ {
+   const char *base_name = lbasename (filename);
+   char *dir_name;
+   char *real_path;
+   char *result;
+ 
+   /* Extract the basename of filename, and return immediately 
+      a copy of filename if it does not contain any directory prefix. */
+   if (base_name == filename)
+     return xstrdup (filename);
+ 
+   dir_name = alloca ((size_t) (base_name - filename + 1));
+   strncpy (dir_name, filename, base_name - filename);
+   dir_name[base_name - filename] = '\000';

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

+   /* Canonicalize the directory prefix, and build the resulting
+      filename. Avoid returning a path which starts with 2 SLASH_CHARS
+      if the canonicalized path is the root path */
+   real_path = gdb_realpath (dir_name);
+   if (strcmp (real_path, SLASH_STRING) == 0)
+     result = concat (SLASH_STRING, base_name, NULL);
+   else
+     result = concat (real_path, SLASH_STRING, base_name, NULL);

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.

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