[7/8] a) Use strcoll() in opendir() and alphasort()

Message ID 20190131130552.30426-8-sebastian.huber@embedded-brains.de
State New
Headers show
Series
  • Synchronize scandir() with latest FreeBSD version
Related show

Commit Message

Sebastian Huber Jan. 31, 2019, 1:05 p.m.
From: ache <ache@FreeBSD.org>


as POSIX 2008 requires. It also matches now how our 'ls' works for years.

b) Remove comment expressed 2 fears:
 1) One just simple describe how strcoll() works in _any_ context,
 not for directories only. Are we plan to remove strcoll() from everything
 just because it is little more complex than strcmp()? I doubt, and
 directories give nothing different here. Moreover, strcoll() used
 in 'ls' for years and nobody complaints yet.

 2) Plain wrong statement about undefined strcoll() behaviour. strcoll()
 always gives predictable results, falling back to strcmp() on any
 trouble, see strcoll(3).

No objections from -current list discussion.
---
 newlib/libc/posix/scandir.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

-- 
2.16.4

Comments

Sebastian Huber Feb. 1, 2019, 6:55 a.m. | #1
On 31/01/2019 14:05, Sebastian Huber wrote:
> From: ache <ache@FreeBSD.org>

>

> as POSIX 2008 requires. It also matches now how our 'ls' works for years.

>

> b) Remove comment expressed 2 fears:

>   1) One just simple describe how strcoll() works in _any_ context,

>   not for directories only. Are we plan to remove strcoll() from everything

>   just because it is little more complex than strcmp()? I doubt, and

>   directories give nothing different here. Moreover, strcoll() used

>   in 'ls' for years and nobody complaints yet.

>

>   2) Plain wrong statement about undefined strcoll() behaviour. strcoll()

>   always gives predictable results, falling back to strcmp() on any

>   trouble, see strcoll(3).

>

> No objections from -current list discussion.

> ---

>   newlib/libc/posix/scandir.c | 7 ++++---

>   1 file changed, 4 insertions(+), 3 deletions(-)

>

> diff --git a/newlib/libc/posix/scandir.c b/newlib/libc/posix/scandir.c

> index 94c583761..13354c05e 100644

> --- a/newlib/libc/posix/scandir.c

> +++ b/newlib/libc/posix/scandir.c

> @@ -142,12 +142,13 @@ fail:

>   

>   /*

>    * Alphabetic order comparison routine for those who want it.

> + * POSIX 2008 requires that alphasort() uses strcoll().

>    */

>   int

> -alphasort (const struct dirent **d1,

> -       const struct dirent **d2)

> +alphasort(const struct dirent **d1, const struct dirent **d2)

>   {

> -       return(strcmp((*d1)->d_name, (*d2)->d_name));

> +

> +	return (strcoll((*d1)->d_name, (*d2)->d_name));

>   }

>   

>   #endif /* ! HAVE_OPENDIR */


After looking into newlib/libc/string/strcoll.c, this patch makes no 
sense. I will not apply it.

-- 
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax     : +49 89 189 47 41-09
E-Mail  : sebastian.huber@embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
Corinna Vinschen Feb. 1, 2019, 9:16 a.m. | #2
On Feb  1 07:55, Sebastian Huber wrote:
> On 31/01/2019 14:05, Sebastian Huber wrote:

> > From: ache <ache@FreeBSD.org>

> > 

> > as POSIX 2008 requires. It also matches now how our 'ls' works for years.

> > 

> > b) Remove comment expressed 2 fears:

> >   1) One just simple describe how strcoll() works in _any_ context,

> >   not for directories only. Are we plan to remove strcoll() from everything

> >   just because it is little more complex than strcmp()? I doubt, and

> >   directories give nothing different here. Moreover, strcoll() used

> >   in 'ls' for years and nobody complaints yet.

> > 

> >   2) Plain wrong statement about undefined strcoll() behaviour. strcoll()

> >   always gives predictable results, falling back to strcmp() on any

> >   trouble, see strcoll(3).

> > 

> > No objections from -current list discussion.

> > ---

> >   newlib/libc/posix/scandir.c | 7 ++++---

> >   1 file changed, 4 insertions(+), 3 deletions(-)

> > 

> > diff --git a/newlib/libc/posix/scandir.c b/newlib/libc/posix/scandir.c

> > index 94c583761..13354c05e 100644

> > --- a/newlib/libc/posix/scandir.c

> > +++ b/newlib/libc/posix/scandir.c

> > @@ -142,12 +142,13 @@ fail:

> >   /*

> >    * Alphabetic order comparison routine for those who want it.

> > + * POSIX 2008 requires that alphasort() uses strcoll().

> >    */

> >   int

> > -alphasort (const struct dirent **d1,

> > -       const struct dirent **d2)

> > +alphasort(const struct dirent **d1, const struct dirent **d2)

> >   {

> > -       return(strcmp((*d1)->d_name, (*d2)->d_name));

> > +

> > +	return (strcoll((*d1)->d_name, (*d2)->d_name));

> >   }

> >   #endif /* ! HAVE_OPENDIR */

> 

> After looking into newlib/libc/string/strcoll.c, this patch makes no sense.

> I will not apply it.


I disagree.  POSIX requires alpphasort to call strcoll, so this is
clearly a bug in newlib's implementation.  Also, what if, at one point,
somebody improves newlib's strcoll?

Cygwin's alphasort uses strcoll as well.  I'm just not quite sure
why Cygwin still has its own implementation.  There's nothing
Cygwin-specific in there, except for the strcmp/strcoll difference.
I guess I'll remove it after the Cygwin 3.0 release.


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat
Sebastian Huber Feb. 1, 2019, 9:40 a.m. | #3
On 01/02/2019 10:16, Corinna Vinschen wrote:
> On Feb  1 07:55, Sebastian Huber wrote:

>> On 31/01/2019 14:05, Sebastian Huber wrote:

>>> From: ache <ache@FreeBSD.org>

>>>

>>> as POSIX 2008 requires. It also matches now how our 'ls' works for years.

>>>

>>> b) Remove comment expressed 2 fears:

>>>    1) One just simple describe how strcoll() works in _any_ context,

>>>    not for directories only. Are we plan to remove strcoll() from everything

>>>    just because it is little more complex than strcmp()? I doubt, and

>>>    directories give nothing different here. Moreover, strcoll() used

>>>    in 'ls' for years and nobody complaints yet.

>>>

>>>    2) Plain wrong statement about undefined strcoll() behaviour. strcoll()

>>>    always gives predictable results, falling back to strcmp() on any

>>>    trouble, see strcoll(3).

>>>

>>> No objections from -current list discussion.

>>> ---

>>>    newlib/libc/posix/scandir.c | 7 ++++---

>>>    1 file changed, 4 insertions(+), 3 deletions(-)

>>>

>>> diff --git a/newlib/libc/posix/scandir.c b/newlib/libc/posix/scandir.c

>>> index 94c583761..13354c05e 100644

>>> --- a/newlib/libc/posix/scandir.c

>>> +++ b/newlib/libc/posix/scandir.c

>>> @@ -142,12 +142,13 @@ fail:

>>>    /*

>>>     * Alphabetic order comparison routine for those who want it.

>>> + * POSIX 2008 requires that alphasort() uses strcoll().

>>>     */

>>>    int

>>> -alphasort (const struct dirent **d1,

>>> -       const struct dirent **d2)

>>> +alphasort(const struct dirent **d1, const struct dirent **d2)

>>>    {

>>> -       return(strcmp((*d1)->d_name, (*d2)->d_name));

>>> +

>>> +	return (strcoll((*d1)->d_name, (*d2)->d_name));

>>>    }

>>>    #endif /* ! HAVE_OPENDIR */

>> After looking into newlib/libc/string/strcoll.c, this patch makes no sense.

>> I will not apply it.

> I disagree.  POSIX requires alpphasort to call strcoll, so this is

> clearly a bug in newlib's implementation.  Also, what if, at one point,

> somebody improves newlib's strcoll?

>

> Cygwin's alphasort uses strcoll as well.  I'm just not quite sure

> why Cygwin still has its own implementation.  There's nothing

> Cygwin-specific in there, except for the strcmp/strcoll difference.

> I guess I'll remove it after the Cygwin 3.0 release.


Ok, I checked everything in modulo the register removal part.

-- 
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax     : +49 89 189 47 41-09
E-Mail  : sebastian.huber@embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

Patch

diff --git a/newlib/libc/posix/scandir.c b/newlib/libc/posix/scandir.c
index 94c583761..13354c05e 100644
--- a/newlib/libc/posix/scandir.c
+++ b/newlib/libc/posix/scandir.c
@@ -142,12 +142,13 @@  fail:
 
 /*
  * Alphabetic order comparison routine for those who want it.
+ * POSIX 2008 requires that alphasort() uses strcoll().
  */
 int
-alphasort (const struct dirent **d1,
-       const struct dirent **d2)
+alphasort(const struct dirent **d1, const struct dirent **d2)
 {
-       return(strcmp((*d1)->d_name, (*d2)->d_name));
+
+	return (strcoll((*d1)->d_name, (*d2)->d_name));
 }
 
 #endif /* ! HAVE_OPENDIR */