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

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

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 302 bytes --]

Hello,

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

2007-04-05  Joel Brobecker  <brobecker@adacore.com>

        * filename_cmp.c (filename_cmp): Improve documentation.

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

Thanks,
-- 
Joel

[-- Attachment #2: filename_cmp.2.diff --]
[-- Type: text/plain, Size: 1377 bytes --]

Index: libiberty/filename_cmp.c
===================================================================
RCS file: /cvs/src/src/libiberty/filename_cmp.c,v
retrieving revision 1.2
diff -u -p -r1.2 filename_cmp.c
--- libiberty/filename_cmp.c	2 Apr 2007 11:20:52 -0000	1.2
+++ libiberty/filename_cmp.c	5 Apr 2007 17:21:47 -0000
@@ -31,12 +31,13 @@
 
 @deftypefn Extension int filename_cmp (const char *@var{s1}, const char *@var{s2})
 
-Return zero if the two paths @var{s1} and @var{s2} are equivalent.
-If not equivalent, the returned value is similar to what strcmp would
-return.  In other words, it returns a negative value if @var{s1} is less
-than @var{s2}, or a positive value if @var{s2} is greater than @var{s2}.
+Return zero if the two file names @var{s1} and @var{s2} are equivalent.
+If not equivalent, the returned value is similar to what @code{strcmp}
+would return.  In other words, it returns a negative value if @var{s1}
+is less than @var{s2}, or a positive value if @var{s2} is greater than
+@var{s2}.
 
-This function does not normalize path names. As a result, this function
+This function does not normalize file names.  As a result, this function
 will treat filenames that are spelled differently as different even in
 the case when the two filenames point to the same underlying file.
 However, it does handle the fact that on DOS-like file systems, forward

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

[DJ Delorie]

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  <brobecker@adacore.com>
> 
>         * filename_cmp.c (filename_cmp): Improve documentation.
> 
> Tested on x86-linux by rebuilding GDB. OK to apply?

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

[Eli Zaretskii]

> Date: Thu, 5 Apr 2007 10:27:20 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> The attached patch incorporates all the documentation changes that Eli
> has recommended.  No code change.
> 
> 2007-04-05  Joel Brobecker  <brobecker@adacore.com>
> 
>         * filename_cmp.c (filename_cmp): Improve documentation.

Thanks, I am happy, as far as the documentation goes.

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

[Joel Brobecker]

> 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  <brobecker@adacore.com>
> > 
> >         * filename_cmp.c (filename_cmp): Improve documentation.
> > 
> > Tested on x86-linux by rebuilding GDB. OK to apply?

-- 
Joel

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

[Joel Brobecker]

Eli,

> > 2007-04-05  Joel Brobecker  <brobecker@adacore.com>
> > 
> >         * 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

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

[Eli Zaretskii]

> Date: Thu, 5 Apr 2007 23:12:18 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gcc-patches@gcc.gnu.org, gdb-patches@sourceware.org
> 
> Eli,
> 
> > > 2007-04-05  Joel Brobecker  <brobecker@adacore.com>
> > > 
> > >         * 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.

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

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

[Daniel Jacobowitz]

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

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

[Eli Zaretskii]

> Date: Sat, 7 Apr 2007 13:35:00 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Joel Brobecker <brobecker@adacore.com>, gcc-patches@gcc.gnu.org,
> 	gdb-patches@sourceware.org
> 
> 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.

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

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

[Joel Brobecker]

> 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

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

[Eli Zaretskii]

> Date: Wed, 11 Apr 2007 09:26:15 +0200
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: Daniel Jacobowitz <drow@false.org>, gcc-patches@gcc.gnu.org,
> 	gdb-patches@sourceware.org
> 
> > 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.

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

[Ian Lance Taylor]

Eli Zaretskii <eliz@gnu.org> writes:

> > Date: Wed, 11 Apr 2007 09:26:15 +0200
> > From: Joel Brobecker <brobecker@adacore.com>
> > Cc: Daniel Jacobowitz <drow@false.org>, gcc-patches@gcc.gnu.org,
> > 	gdb-patches@sourceware.org
> > 
> > > 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

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

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 553 bytes --]

> 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  <brobecker@adacore.com>

	* 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

[-- Attachment #2: tolower.diff --]
[-- Type: text/plain, Size: 1407 bytes --]

Index: filename_cmp.c
===================================================================
RCS file: /cvs/src/src/libiberty/filename_cmp.c,v
retrieving revision 1.3
diff -u -p -r1.3 filename_cmp.c
--- filename_cmp.c	6 Apr 2007 06:01:54 -0000	1.3
+++ filename_cmp.c	1 May 2007 22:09:55 -0000
@@ -24,8 +24,8 @@
 #include <string.h>
 #endif
 
-#include <ctype.h>
 #include "filenames.h"
+#include "safe-ctype.h"
 
 /*
 
@@ -55,8 +55,8 @@ filename_cmp (const char *s1, const char
 #else
   for (;;)
     {
-      int c1 = tolower (*s1);
-      int c2 = tolower (*s2);
+      int c1 = TOLOWER (*s1);
+      int c2 = TOLOWER (*s2);
 
       /* On DOS-based file systems, the '/' and the '\' are equivalent.  */
       if (c1 == '/')
Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/libiberty/Makefile.in,v
retrieving revision 1.86
diff -u -p -r1.86 Makefile.in
--- Makefile.in	29 Mar 2007 21:03:48 -0000	1.86
+++ Makefile.in	1 May 2007 22:09:55 -0000
@@ -651,7 +651,8 @@ $(CONFIGURED_OFILES): stamp-picdir
 	else true; fi
 	$(COMPILE.c) $(srcdir)/fibheap.c $(OUTPUT_OPTION)
 
-./filename_cmp.o: $(srcdir)/filename_cmp.c $(INCDIR)/filenames.h
+./filename_cmp.o: $(srcdir)/filename_cmp.c $(INCDIR)/filenames.h \
+	$(INCDIR)/safe-ctype.h
 	if [ x"$(PICFLAG)" != x ]; then \
 	  $(COMPILE.c) $(PICFLAG) $(srcdir)/filename_cmp.c -o pic/$@; \
 	else true; fi

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

[Ian Lance Taylor]

Joel Brobecker <brobecker@adacore.com> writes:

> 2007-05-03  Joel Brobecker  <brobecker@adacore.com>
> 
> 	* 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

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

[Joel Brobecker]

> > 2007-05-03  Joel Brobecker  <brobecker@adacore.com>
> > 
> > 	* 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

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

[Eli Zaretskii]

> Date: Thu, 3 May 2007 08:36:17 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, drow@false.org, gcc-patches@gcc.gnu.org,
> 	gdb-patches@sourceware.org
> 
> > 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  <brobecker@adacore.com>
> 
> 	* 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.