[PATCH 1/8] General further adoption of SPDX licensing ID tags.

[PATCH 1/8] General further adoption of SPDX licensing ID tags.

[Sebastian Huber]

From: pfg <pfg@FreeBSD.org>

Mainly focus on files that use BSD 3-Clause license.

The Software Package Data Exchange (SPDX) group provides a specification
to make it easier for automated tools to detect and summarize well known
opensource licenses. We are gradually adopting the specification, noting
that the tags are considered only advisory and do not, in any way,
superceed or replace the license texts.

Special thanks to Wind River for providing access to "The Duke of
Highlander" tool: an older (2014) run over FreeBSD tree was useful as a
starting point.
---
 newlib/libc/posix/scandir.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/newlib/libc/posix/scandir.c b/newlib/libc/posix/scandir.c
index d95cff378..57b96827e 100644
--- a/newlib/libc/posix/scandir.c
+++ b/newlib/libc/posix/scandir.c
@@ -1,6 +1,8 @@
 #ifndef HAVE_OPENDIR
 
 /*
+ * SPDX-License-Identifier: BSD-3-Clause
+ *
  * Copyright (c) 1983 Regents of the University of California.
  * All rights reserved.
  *
-- 
2.16.4

[PATCH 4/8] Clean up the vcs ID strings

[Sebastian Huber]

From: jhb <jhb@FreeBSD.org>

in libc's gen/ directory.

- Move CSRG IDs into __SCCSID().
- When a file has been copied, consistently use 'From: <tag>' for strings
  referencing the version of the source file copied from in the license
  block comment.
- Some of the 'From:' tags were using $FreeBSD$ that was being expanded on
  each checkout.  Fix those to hardcode the FreeBSD tag from the file that
  was copied at the time of the copy.
- When multiple strings are present list them in "chronological" order,
  so CSRG (__SCCSID) before FreeBSD (__FBSDID).  If a file came from
  OtherBSD and contains a CSRG ID from the OtherBSD file, use the order
  CSRG -> OtherBSD -> FreeBSD.

Reviewed by:	imp
Differential Revision:	https://reviews.freebsd.org/D15831
---
 newlib/libc/posix/scandir.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/newlib/libc/posix/scandir.c b/newlib/libc/posix/scandir.c
index 2f00d3d92..97a16cf7b 100644
--- a/newlib/libc/posix/scandir.c
+++ b/newlib/libc/posix/scandir.c
@@ -31,9 +31,8 @@
  * SUCH DAMAGE.
  */
 
-#if defined(LIBC_SCCS) && !defined(lint)
-static char sccsid[] = "@(#)scandir.c	5.10 (Berkeley) 2/23/91";
-#endif /* LIBC_SCCS and not lint */
+#include <sys/cdefs.h>
+__SCCSID("@(#)scandir.c	8.3 (Berkeley) 1/2/94");
 
 /*
  * Scan the directory dirname calling select to make a list of selected
-- 
2.16.4

[PATCH 8/8] scandir: Add support for struct dirent::d_type

[Sebastian Huber]

Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>
---
 newlib/libc/posix/scandir.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/newlib/libc/posix/scandir.c b/newlib/libc/posix/scandir.c
index 13354c05e..e2f2254a6 100644
--- a/newlib/libc/posix/scandir.c
+++ b/newlib/libc/posix/scandir.c
@@ -33,7 +33,7 @@
 
 #include <sys/cdefs.h>
 __SCCSID("@(#)scandir.c	8.3 (Berkeley) 1/2/94");
-__FBSDID("$FreeBSD$");
+__FBSDID("$FreeBSD: head/lib/libc/gen/scandir.c 335898 2018-07-03 17:31:45Z jhb $");
 
 /*
  * Scan the directory dirname calling select to make a list of selected
@@ -95,6 +95,9 @@ scandir(const char *dirname, struct dirent ***namelist,
 		if (p == NULL)
 			goto fail;
 		p->d_ino = d->d_ino;
+#ifdef DT_UNKNOWN
+		p->d_type = d->d_type;
+#endif
 		p->d_reclen = d->d_reclen;
 #ifdef _DIRENT_HAVE_D_NAMLEN
 		p->d_namlen = d->d_namlen;
-- 
2.16.4

[PATCH 6/8] scandir(3) previously used st_size

[Sebastian Huber]

From: das <das@FreeBSD.org>

to obtain an initial estimate of the array length needed to store all
the directory entries. Although BSD has historically guaranteed that
st_size is the size of the directory file, POSIX does not, and more to
the point, some recent filesystems such as ZFS use st_size to mean
something else.

The fix is to not stat the directory at all, set the initial
array size to 32 entries, and realloc it in powers of 2 if that
proves insufficient.

PR:	113668
---
 newlib/libc/posix/scandir.c | 84 +++++++++++++++++----------------------------
 1 file changed, 31 insertions(+), 53 deletions(-)

diff --git a/newlib/libc/posix/scandir.c b/newlib/libc/posix/scandir.c
index 8404cd0de..94c583761 100644
--- a/newlib/libc/posix/scandir.c
+++ b/newlib/libc/posix/scandir.c
@@ -42,8 +42,6 @@ __FBSDID("$FreeBSD$");
  * struct dirent (through namelist). Returns -1 if there were any errors.
  */
 
-#include <sys/types.h>
-#include <sys/stat.h>
 #include <stddef.h>
 #include <dirent.h>
 #include <stdlib.h>
@@ -71,41 +69,22 @@ scandir(const char *dirname, struct dirent ***namelist,
     int (*select)(const struct dirent *), int (*dcomp)(const struct dirent **,
 	const struct dirent **))
 {
-	struct dirent *d, *p, **names;
-	size_t nitems;
-	struct stat stb;
-	long arraysz;
+	struct dirent *d, *p, **names = NULL;
+	size_t arraysz, numitems;
 	DIR *dirp;
-	int successful = 0;
-	int rc = 0;
-
-	dirp = NULL;
-	names = NULL;
 
 	if ((dirp = opendir(dirname)) == NULL)
 		return(-1);
 #ifdef HAVE_DD_LOCK
 	__lock_acquire_recursive(dirp->dd_lock);
 #endif
-	if (fstat(dirp->dd_fd, &stb) < 0)
-		goto cleanup;
-
-	/*
- 	 * If there were no directory entries, then bail.
- 	 */
-	if (stb.st_size == 0)
-		goto cleanup;
 
-	/*
-	 * estimate the array size by taking the size of the directory file
-	 * and dividing it by a multiple of the minimum size entry. 
-	 */
-	arraysz = (stb.st_size / 24);
+	numitems = 0;
+	arraysz = 32;	/* initial estimate of the array size */
 	names = (struct dirent **)malloc(arraysz * sizeof(struct dirent *));
 	if (names == NULL)
-		goto cleanup;
+		goto fail;
 
-	nitems = 0;
 	while ((d = readdir(dirp)) != NULL) {
 		if (select != NULL && !(*select)(d))
 			continue;	/* just selected names */
@@ -114,7 +93,7 @@ scandir(const char *dirname, struct dirent ***namelist,
 		 */
 		p = (struct dirent *)malloc(DIRSIZ(d));
 		if (p == NULL)
-			goto cleanup;
+			goto fail;
 		p->d_ino = d->d_ino;
 		p->d_reclen = d->d_reclen;
 #ifdef _DIRENT_HAVE_D_NAMLEN
@@ -127,39 +106,38 @@ scandir(const char *dirname, struct dirent ***namelist,
 		 * Check to make sure the array has space left and
 		 * realloc the maximum size.
 		 */
-		if (++nitems >= arraysz) {
-			if (fstat(dirp->dd_fd, &stb) < 0)
-				goto cleanup;
-			arraysz = stb.st_size / 12;
-			names = (struct dirent **)reallocf((char *)names,
-				arraysz * sizeof(struct dirent *));
-			if (names == NULL)
-				goto cleanup;
+		if (numitems >= arraysz) {
+			struct dirent **names2;
+
+			names2 = reallocarray(names, arraysz,
+			    2 * sizeof(struct dirent *));
+			if (names2 == NULL) {
+				free(p);
+				goto fail;
+			}
+			names = names2;
+			arraysz *= 2;
 		}
-		names[nitems-1] = p;
+		names[numitems++] = p;
 	}
-	successful = 1;
-cleanup:
 	closedir(dirp);
-	if (successful) {
-		if (nitems && dcomp != NULL)
-			qsort(names, nitems, sizeof(struct dirent *), (void *)dcomp);
-		*namelist = names;
-		rc = nitems;
-	} else {  /* We were unsuccessful, clean up storage and return -1.  */
-		if ( names ) {
-			int i;
-			for (i=0; i < nitems; i++ )
-				free( names[i] );
-			free( names );
-		}
-		rc = -1;
-	}
+	if (numitems && dcomp != NULL)
+		qsort(names, numitems, sizeof(struct dirent *), (void *)dcomp);
+	*namelist = names;
+#ifdef HAVE_DD_LOCK
+	__lock_release_recursive(dirp->dd_lock);
+#endif
+	return (numitems);
 
+fail:
+	while (numitems > 0)
+		free(names[--numitems]);
+	free(names);
+	closedir(dirp);
 #ifdef HAVE_DD_LOCK
 	__lock_release_recursive(dirp->dd_lock);
 #endif
-	return(rc);
+	return (-1);
 }
 
 /*
-- 
2.16.4

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

[Sebastian Huber]

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 */
-- 
2.16.4

[PATCH 3/8] Renumber copyright clause 4

[Sebastian Huber]

From: imp <imp@FreeBSD.org>

Renumber cluase 4 to 3, per what everybody else did when BSD granted
them permission to remove clause 3. My insistance on keeping the same
numbering for legal reasons is too pedantic, so give up on that point.

Submitted by:	Jan Schaumann <jschauma@stevens.edu>
Pull Request:	https://github.com/freebsd/freebsd/pull/96
---
 newlib/libc/posix/scandir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/newlib/libc/posix/scandir.c b/newlib/libc/posix/scandir.c
index 20c1d6d4d..2f00d3d92 100644
--- a/newlib/libc/posix/scandir.c
+++ b/newlib/libc/posix/scandir.c
@@ -14,7 +14,7 @@
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
  *    documentation and/or other materials provided with the distribution.
- * 4. Neither the name of the University nor the names of its contributors
+ * 3. Neither the name of the University nor the names of its contributors
  *    may be used to endorse or promote products derived from this software
  *    without specific prior written permission.
  *
-- 
2.16.4

[PATCH 0/8] Synchronize scandir() with latest FreeBSD version

[Sebastian Huber]

Sebastian Huber (2):
  scandir: Update copyright notice from FreeBSD
  scandir: Add support for struct dirent::d_type

ache (1):
  a) Use strcoll() in opendir() and alphasort()

das (1):
  scandir(3) previously used st_size

imp (1):
  Renumber copyright clause 4

jhb (1):
  Clean up the vcs ID strings

obrien (1):
  Remove __P and convert to ANSI prototypes.

pfg (1):
  General further adoption of SPDX licensing ID tags.

 newlib/libc/posix/scandir.c | 120 ++++++++++++++++++--------------------------
 1 file changed, 50 insertions(+), 70 deletions(-)

-- 
2.16.4

[PATCH 5/8] Remove __P and convert to ANSI prototypes.

[Sebastian Huber]

From: obrien <obrien@FreeBSD.org>

* Remove 'register'. (some functions had 7+ register functions...)
* Fix SCM ID's.
---
 newlib/libc/posix/scandir.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/newlib/libc/posix/scandir.c b/newlib/libc/posix/scandir.c
index 97a16cf7b..8404cd0de 100644
--- a/newlib/libc/posix/scandir.c
+++ b/newlib/libc/posix/scandir.c
@@ -33,6 +33,7 @@
 
 #include <sys/cdefs.h>
 __SCCSID("@(#)scandir.c	8.3 (Berkeley) 1/2/94");
+__FBSDID("$FreeBSD$");
 
 /*
  * Scan the directory dirname calling select to make a list of selected
@@ -64,18 +65,14 @@ __SCCSID("@(#)scandir.c	8.3 (Berkeley) 1/2/94");
     (offsetof (struct dirent, d_name) + ((strlen((dp)->d_name)+1 + 3) &~ 3))
 #endif
 
-#ifndef __P
-#define __P(args) ()
-#endif
 
 int
-scandir (const char *dirname,
-	struct dirent ***namelist,
-	int (*select) __P((const struct dirent *)),
-	int (*dcomp) __P((const struct dirent **, const struct dirent **)))
+scandir(const char *dirname, struct dirent ***namelist,
+    int (*select)(const struct dirent *), int (*dcomp)(const struct dirent **,
+	const struct dirent **))
 {
-	register struct dirent *d, *p, **names;
-	register size_t nitems;
+	struct dirent *d, *p, **names;
+	size_t nitems;
 	struct stat stb;
 	long arraysz;
 	DIR *dirp;
-- 
2.16.4

[PATCH 2/8] scandir: Update copyright notice from FreeBSD

[Sebastian Huber]

Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>
---
 newlib/libc/posix/scandir.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/newlib/libc/posix/scandir.c b/newlib/libc/posix/scandir.c
index 57b96827e..20c1d6d4d 100644
--- a/newlib/libc/posix/scandir.c
+++ b/newlib/libc/posix/scandir.c
@@ -1,10 +1,10 @@
 #ifndef HAVE_OPENDIR
 
-/*
+/*-
  * SPDX-License-Identifier: BSD-3-Clause
  *
- * Copyright (c) 1983 Regents of the University of California.
- * All rights reserved.
+ * Copyright (c) 1983, 1993
+ *	The Regents of the University of California.  All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
-- 
2.16.4

Re: [PATCH 5/8] Remove __P and convert to ANSI prototypes.

[Craig Howland]

On 1/31/19 8:05 AM, Sebastian Huber wrote:
> From: obrien <obrien@FreeBSD.org>
>
> * Remove 'register'. (some functions had 7+ register functions...)
> * Fix SCM ID's.
> ---
>   newlib/libc/posix/scandir.c | 15 ++++++---------
>   1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/newlib/libc/posix/scandir.c b/newlib/libc/posix/scandir.c
> index 97a16cf7b..8404cd0de 100644
> --- a/newlib/libc/posix/scandir.c
> +++ b/newlib/libc/posix/scandir.c
> @@ -33,6 +33,7 @@
>   
>   #include <sys/cdefs.h>
>   __SCCSID("@(#)scandir.c	8.3 (Berkeley) 1/2/94");
> +__FBSDID("$FreeBSD$");
>   
>   /*
>    * Scan the directory dirname calling select to make a list of selected
> @@ -64,18 +65,14 @@ __SCCSID("@(#)scandir.c	8.3 (Berkeley) 1/2/94");
>       (offsetof (struct dirent, d_name) + ((strlen((dp)->d_name)+1 + 3) &~ 3))
>   #endif
>   
> -#ifndef __P
> -#define __P(args) ()
> -#endif
>   
>   int
> -scandir (const char *dirname,
> -	struct dirent ***namelist,
> -	int (*select) __P((const struct dirent *)),
> -	int (*dcomp) __P((const struct dirent **, const struct dirent **)))
> +scandir(const char *dirname, struct dirent ***namelist,
> +    int (*select)(const struct dirent *), int (*dcomp)(const struct dirent **,
> +	const struct dirent **))
>   {
> -	register struct dirent *d, *p, **names;
> -	register size_t nitems;
> +	struct dirent *d, *p, **names;
> +	size_t nitems;
>   	struct stat stb;
>   	long arraysz;
>   	DIR *dirp;
      Why?  This seems a step backwards, as the coder is giving a recommendation 
to the compiler, presumably based on the coder's knowledge of the algorithm.  
register can effectively be ignored by compilers (C99 section 6.7.1), so there 
is no harm in having them. In this particular case there are 9 variables 
declared and register is put on 4 of them.  Even if you put stock into the 
general complaint of 'some functions had 7+ register functions', it is not so 
applicable to 4.
       I do not support the complaint as a general rule, anyway.  If there were 
a function with 30 variables and 8 had register, that could be fine.  7 out of 
7, perhaps then reasonable to wonder.  One of the hallmarks of RISC is lots of 
registers.  Plenty more than 7+, even, should generally be available on most 
platforms.  (32 GPRs is pretty common, minus a few for ABI purposes leaves 
perhaps a couple dozen.)  Now if this had been brought up back in 1994 when this 
file is dated, then 4 register would have been a more interesting discussion 
(M68k only had 8 GPR, etc.), but not now.
      I understand the desire to stay in sync with FreeBSD, but does that 
include bad decisions?  I suppose perhaps it should--but only with discussion.
                 Craig

Re: [PATCH 5/8] Remove __P and convert to ANSI prototypes.

[Corinna Vinschen]

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

On Jan 31 12:05, Craig Howland wrote:
> On 1/31/19 8:05 AM, Sebastian Huber wrote:
> > From: obrien <obrien@FreeBSD.org>
> > 
> > * Remove 'register'. (some functions had 7+ register functions...)
> > * Fix SCM ID's.
> > ---
> >   newlib/libc/posix/scandir.c | 15 ++++++---------
> >   1 file changed, 6 insertions(+), 9 deletions(-)
> > 
> > diff --git a/newlib/libc/posix/scandir.c b/newlib/libc/posix/scandir.c
> > index 97a16cf7b..8404cd0de 100644
> > --- a/newlib/libc/posix/scandir.c
> > +++ b/newlib/libc/posix/scandir.c
> > @@ -33,6 +33,7 @@
> >   #include <sys/cdefs.h>
> >   __SCCSID("@(#)scandir.c	8.3 (Berkeley) 1/2/94");
> > +__FBSDID("$FreeBSD$");
> >   /*
> >    * Scan the directory dirname calling select to make a list of selected
> > @@ -64,18 +65,14 @@ __SCCSID("@(#)scandir.c	8.3 (Berkeley) 1/2/94");
> >       (offsetof (struct dirent, d_name) + ((strlen((dp)->d_name)+1 + 3) &~ 3))
> >   #endif
> > -#ifndef __P
> > -#define __P(args) ()
> > -#endif
> >   int
> > -scandir (const char *dirname,
> > -	struct dirent ***namelist,
> > -	int (*select) __P((const struct dirent *)),
> > -	int (*dcomp) __P((const struct dirent **, const struct dirent **)))
> > +scandir(const char *dirname, struct dirent ***namelist,
> > +    int (*select)(const struct dirent *), int (*dcomp)(const struct dirent **,
> > +	const struct dirent **))
> >   {
> > -	register struct dirent *d, *p, **names;
> > -	register size_t nitems;
> > +	struct dirent *d, *p, **names;
> > +	size_t nitems;
> >   	struct stat stb;
> >   	long arraysz;
> >   	DIR *dirp;
>      Why?  This seems a step backwards, as the coder is giving a
> recommendation to the compiler, presumably based on the coder's knowledge of
> the algorithm.  register can effectively be ignored by compilers (C99
> section 6.7.1), so there is no harm in having them. In this particular case
> there are 9 variables declared and register is put on 4 of them.  Even if
> you put stock into the general complaint of 'some functions had 7+ register
> functions', it is not so applicable to 4.
>       I do not support the complaint as a general rule, anyway.  If there
> were a function with 30 variables and 8 had register, that could be fine.  7
> out of 7, perhaps then reasonable to wonder.  One of the hallmarks of RISC
> is lots of registers.  Plenty more than 7+, even, should generally be
> available on most platforms.  (32 GPRs is pretty common, minus a few for ABI
> purposes leaves perhaps a couple dozen.)  Now if this had been brought up
> back in 1994 when this file is dated, then 4 register would have been a more
> interesting discussion (M68k only had 8 GPR, etc.), but not now.
>      I understand the desire to stay in sync with FreeBSD, but does that
> include bad decisions?  I suppose perhaps it should--but only with
> discussion.
>                 Craig

If the developer guesses on what should be done to optimize, rather then
letting the compiler decide what to optimize, the developer is usually
wrong.  That's what -O2 is for, no?  On a RISC with lots of registers
I expect the compiler to use them wisely.


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/8] Synchronize scandir() with latest FreeBSD version

[Corinna Vinschen]

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

On Jan 31 14:05, Sebastian Huber wrote:
> Sebastian Huber (2):
>   scandir: Update copyright notice from FreeBSD
>   scandir: Add support for struct dirent::d_type
> 
> ache (1):
>   a) Use strcoll() in opendir() and alphasort()
> 
> das (1):
>   scandir(3) previously used st_size
> 
> imp (1):
>   Renumber copyright clause 4
> 
> jhb (1):
>   Clean up the vcs ID strings
> 
> obrien (1):
>   Remove __P and convert to ANSI prototypes.
> 
> pfg (1):
>   General further adoption of SPDX licensing ID tags.
> 
>  newlib/libc/posix/scandir.c | 120 ++++++++++++++++++--------------------------
>  1 file changed, 50 insertions(+), 70 deletions(-)
> 
> -- 
> 2.16.4

Apart from any discussion in terms of path 5, the patchset looks
good to me.


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/8] Remove __P and convert to ANSI prototypes.

[Sebastian Huber]

On 31/01/2019 20:52, Corinna Vinschen wrote:
> On Jan 31 12:05, Craig Howland wrote:
>> On 1/31/19 8:05 AM, Sebastian Huber wrote:
>>> From: obrien<obrien@FreeBSD.org>
>>>
>>> * Remove 'register'. (some functions had 7+ register functions...)
>>> * Fix SCM ID's.
>>> ---
>>>    newlib/libc/posix/scandir.c | 15 ++++++---------
>>>    1 file changed, 6 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/newlib/libc/posix/scandir.c b/newlib/libc/posix/scandir.c
>>> index 97a16cf7b..8404cd0de 100644
>>> --- a/newlib/libc/posix/scandir.c
>>> +++ b/newlib/libc/posix/scandir.c
>>> @@ -33,6 +33,7 @@
>>>    #include <sys/cdefs.h>
>>>    __SCCSID("@(#)scandir.c	8.3 (Berkeley) 1/2/94");
>>> +__FBSDID("$FreeBSD$");
>>>    /*
>>>     * Scan the directory dirname calling select to make a list of selected
>>> @@ -64,18 +65,14 @@ __SCCSID("@(#)scandir.c	8.3 (Berkeley) 1/2/94");
>>>        (offsetof (struct dirent, d_name) + ((strlen((dp)->d_name)+1 + 3) &~ 3))
>>>    #endif
>>> -#ifndef __P
>>> -#define __P(args) ()
>>> -#endif
>>>    int
>>> -scandir (const char *dirname,
>>> -	struct dirent ***namelist,
>>> -	int (*select) __P((const struct dirent *)),
>>> -	int (*dcomp) __P((const struct dirent **, const struct dirent **)))
>>> +scandir(const char *dirname, struct dirent ***namelist,
>>> +    int (*select)(const struct dirent *), int (*dcomp)(const struct dirent **,
>>> +	const struct dirent **))
>>>    {
>>> -	register struct dirent *d, *p, **names;
>>> -	register size_t nitems;
>>> +	struct dirent *d, *p, **names;
>>> +	size_t nitems;
>>>    	struct stat stb;
>>>    	long arraysz;
>>>    	DIR *dirp;
>>       Why?  This seems a step backwards, as the coder is giving a
>> recommendation to the compiler, presumably based on the coder's knowledge of
>> the algorithm.  register can effectively be ignored by compilers (C99
>> section 6.7.1), so there is no harm in having them. In this particular case
>> there are 9 variables declared and register is put on 4 of them.  Even if
>> you put stock into the general complaint of 'some functions had 7+ register
>> functions', it is not so applicable to 4.
>>        I do not support the complaint as a general rule, anyway.  If there
>> were a function with 30 variables and 8 had register, that could be fine.  7
>> out of 7, perhaps then reasonable to wonder.  One of the hallmarks of RISC
>> is lots of registers.  Plenty more than 7+, even, should generally be
>> available on most platforms.  (32 GPRs is pretty common, minus a few for ABI
>> purposes leaves perhaps a couple dozen.)  Now if this had been brought up
>> back in 1994 when this file is dated, then 4 register would have been a more
>> interesting discussion (M68k only had 8 GPR, etc.), but not now.
>>       I understand the desire to stay in sync with FreeBSD, but does that
>> include bad decisions?  I suppose perhaps it should--but only with
>> discussion.
>>                  Craig
> If the developer guesses on what should be done to optimize, rather then
> letting the compiler decide what to optimize, the developer is usually
> wrong.  That's what -O2 is for, no?  On a RISC with lots of registers
> I expect the compiler to use them wisely.

I don't really care about the register keyword. I will remove this part 
of the patch.

What about the __P removal?

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

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

[Sebastian Huber]

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.

Re: [PATCH 5/8] Remove __P and convert to ANSI prototypes.

[Corinna Vinschen]

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

On Feb  1 07:48, Sebastian Huber wrote:
> On 31/01/2019 20:52, Corinna Vinschen wrote:
> > On Jan 31 12:05, Craig Howland wrote:
> > > On 1/31/19 8:05 AM, Sebastian Huber wrote:
> > > > From: obrien<obrien@FreeBSD.org>
> > > > 
> > > > * Remove 'register'. (some functions had 7+ register functions...)
> > > > * Fix SCM ID's.
> > > > ---
> > > >    newlib/libc/posix/scandir.c | 15 ++++++---------
> > > >    1 file changed, 6 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/newlib/libc/posix/scandir.c b/newlib/libc/posix/scandir.c
> > > > index 97a16cf7b..8404cd0de 100644
> > > > --- a/newlib/libc/posix/scandir.c
> > > > +++ b/newlib/libc/posix/scandir.c
> > > > @@ -33,6 +33,7 @@
> > > >    #include <sys/cdefs.h>
> > > >    __SCCSID("@(#)scandir.c	8.3 (Berkeley) 1/2/94");
> > > > +__FBSDID("$FreeBSD$");
> > > >    /*
> > > >     * Scan the directory dirname calling select to make a list of selected
> > > > @@ -64,18 +65,14 @@ __SCCSID("@(#)scandir.c	8.3 (Berkeley) 1/2/94");
> > > >        (offsetof (struct dirent, d_name) + ((strlen((dp)->d_name)+1 + 3) &~ 3))
> > > >    #endif
> > > > -#ifndef __P
> > > > -#define __P(args) ()
> > > > -#endif
> > > >    int
> > > > -scandir (const char *dirname,
> > > > -	struct dirent ***namelist,
> > > > -	int (*select) __P((const struct dirent *)),
> > > > -	int (*dcomp) __P((const struct dirent **, const struct dirent **)))
> > > > +scandir(const char *dirname, struct dirent ***namelist,
> > > > +    int (*select)(const struct dirent *), int (*dcomp)(const struct dirent **,
> > > > +	const struct dirent **))
> > > >    {
> > > > -	register struct dirent *d, *p, **names;
> > > > -	register size_t nitems;
> > > > +	struct dirent *d, *p, **names;
> > > > +	size_t nitems;
> > > >    	struct stat stb;
> > > >    	long arraysz;
> > > >    	DIR *dirp;
> > >       Why?  This seems a step backwards, as the coder is giving a
> > > recommendation to the compiler, presumably based on the coder's knowledge of
> > > [...]
> > I expect the compiler to use them wisely.
> 
> I don't really care about the register keyword. I will remove this part of
> the patch.
> 
> What about the __P removal?

Looks good.


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Corinna Vinschen]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Sebastian Huber]

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.

Re: [PATCH 5/8] Remove __P and convert to ANSI prototypes.

[Craig Howland]

On 2/1/19 4:09 AM, Corinna Vinschen wrote:
> On Feb  1 07:48, Sebastian Huber wrote:
>> On 31/01/2019 20:52, Corinna Vinschen wrote:
>>> On Jan 31 12:05, Craig Howland wrote:
>>>> On 1/31/19 8:05 AM, Sebastian Huber wrote:
>>>>> From: obrien<obrien@FreeBSD.org>
>>>>>
>>>>> * Remove 'register'. (some functions had 7+ register functions...)
>>>>> * Fix SCM ID's.
>>>>> ---
>>>>>     newlib/libc/posix/scandir.c | 15 ++++++---------
>>>>>     1 file changed, 6 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/newlib/libc/posix/scandir.c b/newlib/libc/posix/scandir.c
>>>>> index 97a16cf7b..8404cd0de 100644
>>>>> --- a/newlib/libc/posix/scandir.c
>>>>> +++ b/newlib/libc/posix/scandir.c
>>>>> @@ -33,6 +33,7 @@
>>>>>     #include <sys/cdefs.h>
>>>>>     __SCCSID("@(#)scandir.c	8.3 (Berkeley) 1/2/94");
>>>>> +__FBSDID("$FreeBSD$");
>>>>>     /*
>>>>>      * Scan the directory dirname calling select to make a list of selected
>>>>> @@ -64,18 +65,14 @@ __SCCSID("@(#)scandir.c	8.3 (Berkeley) 1/2/94");
>>>>>         (offsetof (struct dirent, d_name) + ((strlen((dp)->d_name)+1 + 3) &~ 3))
>>>>>     #endif
>>>>> -#ifndef __P
>>>>> -#define __P(args) ()
>>>>> -#endif
>>>>>     int
>>>>> -scandir (const char *dirname,
>>>>> -	struct dirent ***namelist,
>>>>> -	int (*select) __P((const struct dirent *)),
>>>>> -	int (*dcomp) __P((const struct dirent **, const struct dirent **)))
>>>>> +scandir(const char *dirname, struct dirent ***namelist,
>>>>> +    int (*select)(const struct dirent *), int (*dcomp)(const struct dirent **,
>>>>> +	const struct dirent **))
>>>>>     {
>>>>> -	register struct dirent *d, *p, **names;
>>>>> -	register size_t nitems;
>>>>> +	struct dirent *d, *p, **names;
>>>>> +	size_t nitems;
>>>>>     	struct stat stb;
>>>>>     	long arraysz;
>>>>>     	DIR *dirp;
>>>>        Why?  This seems a step backwards, as the coder is giving a
>>>> recommendation to the compiler, presumably based on the coder's knowledge of
>>>> [...]
>>> I expect the compiler to use them wisely.
>> I don't really care about the register keyword. I will remove this part of
>> the patch.
>>
>> What about the __P removal?
> Looks good.
>
>
> Corinna
>
      I agree with Coinna that __P removal is good.  (Goes with the big ANSI 
cleanup done last year.)
      I had never done so before, but I found how GCC uses the "register" 
keyword, and it was not what I had expected.  In short (skipping some 
specialized standard-extension sub-cases), from the 7.2.0 manual, GCC uses 
register only with -O0 (putting non-"register"-qualified variables in the 
stack), ignoring it otherwise.  (I had previously assumed that when the 
optimizer was on that a register attribute would add weighting to the 
optimizer's choices.)  Given this, I think it is more important to keep them for 
those times when someone might choose to use -O0.  So I'm glad you did decide to 
remove this part of the patch.
                 Craig