Re: RFA: Use lrealpath instead of gdb_realpath

[Daniel Jacobowitz <drow@false.org>]

On Sun, May 29, 2005 at 11:36:27AM +0300, Eli Zaretskii wrote:
> As your patch clearly shows, gdb_realpath is _not_ identical to
> lrealpath.  For example, it doesn't have the Windows code that calls
> GetFullPathName.  So we are not replacing a duplicated function, not
> really.

I don't know how much clearer I can get.  They were duplicated
functions.  I can say that with great certainty, since I used
gdb_realpath as a basis for lrealpath when I needed its functionality
in libiberty.  They have diverged over time.  That creates a
maintenance burden - this particular one in fact.

> Let me explain my concerns about using lrealpath in its current shape:
> 
>  . The Windows code in lrealpath converts all forward slashes to
>    backslashes, which I think is a bad idea.  By contrast, the current
>    gdb_realpath would leave existing forward slashes alone.  That is
>    an incompatible change, and my gut feeling is that we should not
>    risk it.  On top of that, I think we always use '/' to generate
>    file names, so with this change we will produce ugly file names
>    like D:\foo\bar/baz.c.

The Windows maintainers disagree with you.  So, for the record, do I.

>  . The Windows code in lrealpath downcases the file name returned by
>    GetFullPathName.  That might be okay for when we need a canonical
>    file name suitable for comparison by strcmp, but in the case of GDB
>    it is IMHO a misservice to the user, since (a) we correctly use
>    FILENAME_CMP to compare file names, and (b) presenting downcased
>    file names to the user is a gratuitous ugliness that we should try
>    to avoid.
> 
>  . GetFullPathName is documented to work even if the path and/or the
>    file name do not exist.  By contrast, both realpath and
>    canonicalize_file_name are documented to fail if some or all of the
>    components of the resulting file name do not exist.  Thus, I submit
>    that the Windows code is not a faithful emulation of the Posix
>    code, and thus could cause subtle bugs/misfeatures.

These were both implemented by Danny Smith.  You were furious with me
for daring to disagree with you on how DJGPP handles filenames; and now
you're critiquing the mingw maintainers for how they choose to handle
filenames?  Please, pardon me if I find that a bit ironic.

>  . lrealpath uses strdup while gdb_realpath uses xstrdup.  Thus, if
>    we use lrealpath, we will behave differently when we run out of
>    memory.

That said, thank you for catching this!  I will not apply the patch.
This is definitely a problem since existing users assume this behavior.
I was asked to make this change when I moved the function into
libiberty, since libiberty functions should not abort on their own
initiative.

It is not worth my time to continue arguing about filename handling;
consider the patch withdrawn indefinitely.

> > --- symtab.c	8 Mar 2005 04:34:44 -0000	1.145
> > +++ symtab.c	28 May 2005 23:13:40 -0000
> > @@ -163,7 +163,7 @@ lookup_symtab (const char *name)
> >      {
> >        full_path = xfullpath (name);
> >        make_cleanup (xfree, full_path);
> > -      real_path = gdb_realpath (name);
> > +      real_path = lrealpath (name);
> >        make_cleanup (xfree, real_path);
> >      }
> 
> Can we please talk about this snippet?  I don't understand the need
> for calling both xfullpath and lrealpath here.  If that's because
> lrealpath might fail if the basename does not exist, I think we should
> call lrealpath first and fall back on xfullpath only after that.

We can't talk about it in the context of a patch replacing gdb_realpath
with lrealpath.  If you'd like to talk about it separately, go ahead.
Having looked at the context, I have no opinion either way.

-- 
Daniel Jacobowitz
CodeSourcery, LLC