[RFA/libiberty] Fix documentation issues in filename_cmp.c

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

[RFA/libiberty] Fix documentation issues in filename_cmp.c

Joel Brobecker
Hello,

The attached patch incorporates all the documentation changes that Eli
has recommended.  No code change.

2007-04-05  Joel Brobecker  <[hidden email]>

        * filename_cmp.c (filename_cmp): Improve documentation.

Tested on x86-linux by rebuilding GDB. OK to apply?

Thanks,
--
Joel

filename_cmp.2.diff (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Fix documentation issues in filename_cmp.c

DJ Delorie-2

I'm OK with it if Eli likes it.

> The attached patch incorporates all the documentation changes that Eli
> has recommended.  No code change.
>
> 2007-04-05  Joel Brobecker  <[hidden email]>
>
>         * filename_cmp.c (filename_cmp): Improve documentation.
>
> Tested on x86-linux by rebuilding GDB. OK to apply?
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Fix documentation issues in filename_cmp.c

Eli Zaretskii
In reply to this post by Joel Brobecker
> Date: Thu, 5 Apr 2007 10:27:20 -0700
> From: Joel Brobecker <[hidden email]>
> Cc: [hidden email]
>
> The attached patch incorporates all the documentation changes that Eli
> has recommended.  No code change.
>
> 2007-04-05  Joel Brobecker  <[hidden email]>
>
>         * filename_cmp.c (filename_cmp): Improve documentation.

Thanks, I am happy, as far as the documentation goes.
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Fix documentation issues in filename_cmp.c

Joel Brobecker
In reply to this post by DJ Delorie-2
> I'm OK with it if Eli likes it.

Thanks! Eli was OK, so I checked this in.

> > The attached patch incorporates all the documentation changes that Eli
> > has recommended.  No code change.
> >
> > 2007-04-05  Joel Brobecker  <[hidden email]>
> >
> >         * filename_cmp.c (filename_cmp): Improve documentation.
> >
> > Tested on x86-linux by rebuilding GDB. OK to apply?

--
Joel
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Fix documentation issues in filename_cmp.c

Joel Brobecker
In reply to this post by Eli Zaretskii
Eli,

> > 2007-04-05  Joel Brobecker  <[hidden email]>
> >
> >         * filename_cmp.c (filename_cmp): Improve documentation.
>
> Thanks, I am happy, as far as the documentation goes.

Am I to understand that you are not happy with the code?

You made two comments about the code:

  1. Fold multiple consecutive slash/backslash characters into
     single slash/backslash;

     To do that without modifying the source filenames, we need to
     allocate some memory locally to manipulate a copy of that filename.
     My take on this is that the file names we have seen, at least in
     the debugging information, have been consistent; and thus this
     enhancement would end up having no actual effect. I would wait
     until we come across a case where this is a problem before
     going that way. This is my opinion, and you may disagree...

     Note that this new function does pave the way for this sort
     of enhancement though, and I promise I will not object to a patch
     that implements your suggestion ;-).

  2. The possible introduction of a bug with certain locales because
     the function used in place of strcasecmp uses tolower.

     For this one, as I said, I don't know, and I'm not sure where
     to start looking for the answer. I'll be happy to modify the
     function body to do it any other way, if someone tells me how.

--
Joel
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Fix documentation issues in filename_cmp.c

Eli Zaretskii
> Date: Thu, 5 Apr 2007 23:12:18 -0700
> From: Joel Brobecker <[hidden email]>
> Cc: [hidden email], [hidden email]
>
> Eli,
>
> > > 2007-04-05  Joel Brobecker  <[hidden email]>
> > >
> > >         * filename_cmp.c (filename_cmp): Improve documentation.
> >
> > Thanks, I am happy, as far as the documentation goes.
>
> Am I to understand that you are not happy with the code?

Well, I did have comments about that, and they were left unresolved ;-)

>   1. Fold multiple consecutive slash/backslash characters into
>      single slash/backslash;
>
>      To do that without modifying the source filenames, we need to
>      allocate some memory locally to manipulate a copy of that filename.

This is a misunderstanding: I didn't mean that we should modify the
source file names.  What I meant is that the comparison should ignore
multiple consecutive slashes, so that, say, "/foo/bar/baz" compares
equal to "/foo//bar////baz" (and to "\foo//bar\/\baz" on Windows).
(However, note that, as Chris pointed out, double slash at the
beginning of a file name, as in "//foo/bar", are significant on
Windows.)

>      My take on this is that the file names we have seen, at least in
>      the debugging information, have been consistent; and thus this
>      enhancement would end up having no actual effect. I would wait
>      until we come across a case where this is a problem before
>      going that way.

I've seen quite a few of such situations, actually.  And in any case,
good engineering doesn't need examples to know what's Right ;-)

>   2. The possible introduction of a bug with certain locales because
>      the function used in place of strcasecmp uses tolower.
>
>      For this one, as I said, I don't know, and I'm not sure where
>      to start looking for the answer.

How about using your function with LANG set to various values?  That
should at least tell us whether strcasecmp and tolower do the same
thing; if they do, there's no problem here.
Reply | Threaded
Open this post in threaded view
|

RE: [RFA/libiberty] Fix documentation issues in filename_cmp.c

Dave Korn
On 06 April 2007 10:06, Eli Zaretskii wrote:


>>   1. Fold multiple consecutive slash/backslash characters into      single
>> slash/backslash;

> (However, note that, as Chris pointed out, double slash at the
> beginning of a file name, as in "//foo/bar", are significant on
> Windows.)

  JFTR, it's a POSIX requirement too:

http://www.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap04.html#tag_04_
11

" A pathname that begins with two successive slashes may be interpreted in an
implementation-defined manner, although more than two leading slashes shall be
treated as a single slash. "


    cheers,
      DaveK
--
Can't think of a witty .sigline today....

Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Fix documentation issues in filename_cmp.c

Daniel Jacobowitz-2
In reply to this post by Eli Zaretskii
On Fri, Apr 06, 2007 at 12:05:54PM +0300, Eli Zaretskii wrote:
> >   2. The possible introduction of a bug with certain locales because
> >      the function used in place of strcasecmp uses tolower.
> >
> >      For this one, as I said, I don't know, and I'm not sure where
> >      to start looking for the answer.
>
> How about using your function with LANG set to various values?  That
> should at least tell us whether strcasecmp and tolower do the same
> thing; if they do, there's no problem here.

For what it's worth, here's what I know about this:

1.  POSIX says:

In the POSIX locale, strcasecmp() and strncasecmp() shall behave as if
the strings had been converted to lowercase and then a byte comparison
performed. The results are unspecified in other locales.

2.  Glibc's strcasecmp uses tolower and honors LC_COLLATE.


Which behavior do we want here, anyway?  I'm not sure...

--
Daniel Jacobowitz
CodeSourcery
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Fix documentation issues in filename_cmp.c

Eli Zaretskii
> Date: Sat, 7 Apr 2007 13:35:00 -0400
> From: Daniel Jacobowitz <[hidden email]>
> Cc: Joel Brobecker <[hidden email]>, [hidden email],
> [hidden email]
>
> In the POSIX locale, strcasecmp() and strncasecmp() shall behave as if
> the strings had been converted to lowercase and then a byte comparison
> performed. The results are unspecified in other locales.

That's what I suspected.

> 2.  Glibc's strcasecmp uses tolower and honors LC_COLLATE.

But the version in libiberty only handles ASCII.

> Which behavior do we want here, anyway?  I'm not sure...

I think we should fold only ASCII characters, since it's consistent
with libiberty's strcasecmp.
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Fix documentation issues in filename_cmp.c

Joel Brobecker
> > 2.  Glibc's strcasecmp uses tolower and honors LC_COLLATE.
>
> But the version in libiberty only handles ASCII.
>
> > Which behavior do we want here, anyway?  I'm not sure...
>
> I think we should fold only ASCII characters, since it's consistent
> with libiberty's strcasecmp.

If we follow your recommendation, I think the best approach is to
use strcasecmp after having changed forward slashes into backward
slashes like I did in my first implementation. That way, we let
strcasecmp deal with the folding.

The downside is that we now end up having to allocate some memory
to hold a copy of the two filenames (in order to do the fixup on
slashes above), whereas we don't need that right now. This is something
that will be needed eventually if someone wants to enhance it to
handle multiple slashes, etc. So adding the couple of string copies
now isn't so bad.

--
Joel
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Fix documentation issues in filename_cmp.c

Joel Brobecker
In reply to this post by Eli Zaretskii
> I've seen quite a few of such situations, actually.  And in any case,
> good engineering doesn't need examples to know what's Right ;-)

OK, if you've seen some cases where it would have helped, then fine.
Otherwise, I've personally been bitten a couple of times after having
done a change that was The Right Thing and yet unnecessary for the
program at hand. The change itself should have had no effect, and
yet I introduced new bugs... Since then, I'm more careful of introducing
only changes that I need.

--
Joel
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Fix documentation issues in filename_cmp.c

Eli Zaretskii
In reply to this post by Joel Brobecker
> Date: Wed, 11 Apr 2007 09:26:15 +0200
> From: Joel Brobecker <[hidden email]>
> Cc: Daniel Jacobowitz <[hidden email]>, [hidden email],
> [hidden email]
>
> > I think we should fold only ASCII characters, since it's consistent
> > with libiberty's strcasecmp.
>
> If we follow your recommendation, I think the best approach is to
> use strcasecmp after having changed forward slashes into backward
> slashes like I did in my first implementation. That way, we let
> strcasecmp deal with the folding.

That's one way, but, as you point out, it has drawbacks.  So my advice
would be to compare individual characters so that A-Za-z compare
case-insensitively.  A simple macro or inline function should be able
to do this.  For example, if you mask the 5th bit, upper-case and
lower-case ASCII will be the same.
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] Fix documentation issues in filename_cmp.c

Ian Lance Taylor-3
Eli Zaretskii <[hidden email]> writes:

> > Date: Wed, 11 Apr 2007 09:26:15 +0200
> > From: Joel Brobecker <[hidden email]>
> > Cc: Daniel Jacobowitz <[hidden email]>, [hidden email],
> > [hidden email]
> >
> > > I think we should fold only ASCII characters, since it's consistent
> > > with libiberty's strcasecmp.
> >
> > If we follow your recommendation, I think the best approach is to
> > use strcasecmp after having changed forward slashes into backward
> > slashes like I did in my first implementation. That way, we let
> > strcasecmp deal with the folding.
>
> That's one way, but, as you point out, it has drawbacks.  So my advice
> would be to compare individual characters so that A-Za-z compare
> case-insensitively.  A simple macro or inline function should be able
> to do this.  For example, if you mask the 5th bit, upper-case and
> lower-case ASCII will be the same.

For locale-independent case conversion, use TOLOWER or TOUPPER in
include/safe-ctype.h.

Ian
Reply | Threaded
Open this post in threaded view
|

[RFA/libiberty] use TOLOWER instead of tolower in filename_cmp.c

Joel Brobecker
> For locale-independent case conversion, use TOLOWER or TOUPPER in
> include/safe-ctype.h.

Thanks for the suggestion, Ian. Now that I'm back, here is a patch
that implements it.

2007-05-03  Joel Brobecker  <[hidden email]>

        * filename_cmp.c: Replace include of ctype.h by include of
        safe-ctype.h.
        (filename_cmp): Use TOLOWER instead of tolower for conversions
        that are locale-independent.
        * Makefile.in (filename_cmp.o): Add dependency on safe-ctype.h.

Tested on x86-linux and x86-windows. No regression.
OK to commit?

Thanks,
--
Joel

tolower.diff (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] use TOLOWER instead of tolower in filename_cmp.c

Ian Lance Taylor-3
Joel Brobecker <[hidden email]> writes:

> 2007-05-03  Joel Brobecker  <[hidden email]>
>
> * filename_cmp.c: Replace include of ctype.h by include of
> safe-ctype.h.
> (filename_cmp): Use TOLOWER instead of tolower for conversions
> that are locale-independent.
> * Makefile.in (filename_cmp.o): Add dependency on safe-ctype.h.

This is OK.  Thanks.

Ian
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] use TOLOWER instead of tolower in filename_cmp.c

Joel Brobecker
> > 2007-05-03  Joel Brobecker  <[hidden email]>
> >
> > * filename_cmp.c: Replace include of ctype.h by include of
> > safe-ctype.h.
> > (filename_cmp): Use TOLOWER instead of tolower for conversions
> > that are locale-independent.
> > * Makefile.in (filename_cmp.o): Add dependency on safe-ctype.h.
>
> This is OK.  Thanks.

Thanks for the quick review! I just checked this in.

--
Joel
Reply | Threaded
Open this post in threaded view
|

Re: [RFA/libiberty] use TOLOWER instead of tolower in filename_cmp.c

Eli Zaretskii
In reply to this post by Joel Brobecker
> Date: Thu, 3 May 2007 08:36:17 -0700
> From: Joel Brobecker <[hidden email]>
> Cc: Eli Zaretskii <[hidden email]>, [hidden email], [hidden email],
> [hidden email]
>
> > For locale-independent case conversion, use TOLOWER or TOUPPER in
> > include/safe-ctype.h.
>
> Thanks for the suggestion, Ian. Now that I'm back, here is a patch
> that implements it.
>
> 2007-05-03  Joel Brobecker  <[hidden email]>
>
> * filename_cmp.c: Replace include of ctype.h by include of
> safe-ctype.h.
> (filename_cmp): Use TOLOWER instead of tolower for conversions
> that are locale-independent.
> * Makefile.in (filename_cmp.o): Add dependency on safe-ctype.h.

Thanks, I'm happy now.