Re: [bug-gnulib] New GNULIB glob module?

[Paul Eggert]

Derek Price <derek@ximbiot.com> writes:

> +#else
> +/* Is there really a case where the getlogin or getlogin_r proto can come 
> from
> +   somewhere other than <unistd.h>?  */
> +# ifdef HAVE_GETLOGIN_R
> +extern int getlogin_r (char *, size_t);
> +# else
> +extern char *getlogin (void);
> +# endif
> +#endif

This comment is somewhat odd, and suggests that you're as troubled by the
code as I am.  How about if you replace the above by the following:

   #if ! HAVE_DECL_GETLOGIN_R
   # if ! HAVE_DECL_GETLOGIN
   char *getlogin (void);
   # endif
   static int
   getlogin_r (char *name, size_t size)
   {
     char *n = getlogin ();
     if (n)
       {
         size_t nlen = strlen (n);
         if (nlen < size)
           {
             memcpy (name, n, nlen + 1);
             return 0;
           }
         errno = ERANGE;
       }
     return -1;
   }
   #endif

and modify the rest of the code to assume that getlogin_r works, and
modify glob.m4 to check for whether getlogin and getlogin_r are
declared, rather than whether they exist.  Or, perhaps better, you can
create a getlogin_r module with the above code, and have the glob
module depend on that.

> +# ifdef HAVE___POSIX_GETPWNAM_R
> +   /* Solaris.  */
> +#  define getpwnam_r(name, bufp, buf, len, res) \
> +    __posix_getpwnam_r (name, bufp, buf, len, res)
> +# endif

I don't see why this is needed.  The Solaris include files already
contain the magic necessary to convert getpwnam_r to
__posix_getpwnam_r, right?  Or is it because that depends on
_POSIX_PTHREAD_SEMANTICS?  If so, why not define that, as follows?  I
don't see why any gnulib-using application would want the nonstandard
Solaris versions of the *_r functions.

--- extensions.m4.~1.6.~        2005-02-23 05:49:36 -0800
+++ extensions.m4       2005-05-24 12:35:48 -0700
@@ -21,6 +21,10 @@ AC_DEFUN([gl_USE_SYSTEM_EXTENSIONS], [
 [/* Enable extensions on Solaris.  */
 #ifndef __EXTENSIONS__
 # undef __EXTENSIONS__
+#endif
+#ifndef _POSIX_PTHREAD_SEMANTICS
+# undef _POSIX_PTHREAD_SEMANTICS
 #endif])
   AC_DEFINE([__EXTENSIONS__])
+  AC_DEFINE([_POSIX_PTHREAD_SEMANTICS])
 ])


> +#if defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE
>  # define HAVE_D_TYPE 1
> -#endif

At this point it would be simpler and clearer if we removed these
lines, and removed all uses of HAVE_D_TYPE.  We can simply substitute
"#ifdef _DIRENT_HAVE_D_TYPE" for "#ifdef HAVE_D_TYPE" later.
(HAVE_STRUCT_DIRENT_D_TYPE isn't relevant for that ifdef.)


Now turning to glob_.h:

> +#if defined _LIBC || HAVE_SYS_CDEFS_H
> +# include <sys/cdefs.h>
> +#endif

Ouch.  I just noticed that this isn't right for the case where glob.h
is installed into /usr/include/glob.h as part of a typical C library
installation.  First, _LIBC won't be defined there.  Second, glibc
can't infringe on the user's name space by depending on HAVE_SYS_CDEFS_H.

One way to work around this is to rewrite it as follows:

#ifndef _SYS_CDEFS_H
# include <sys/cdefs.h>
#endif

where glob.m4 defines _SYS_CDEFS_H if sys/cdefs.h is missing.
This differs from the usual gnulib convention, but it might
be simplest.

Another possibility is to do "mkdir sys; echo '' >sys/cdefs.h" if
<sys/cdefs.h> is missing, so that "#include <sys/cdefs.h>" always
works.  This is a bit yucky on the build side, though it simplifies
the source code.


> +/* We need `size_t' for the following definitions.  */
> +#ifdef _LIBC
> +__BEGIN_DECLS

The comment seems a bit misplaced now.

How about if we build on my previous suggestion above, as follows?

#ifndef _SYS_CDEFS_H
# include <sys/cdefs.h>
#endif
#ifndef __BEGIN_DECLS
# define __BEGIN_DECLS
# define __END_DECLS
# define __THROW
#endif

and remove the existing "define __THROW".  This makes it clearer that
these three macros are defined by sys/cdefs.h.  Then we can keep the
__BEGIN_DECLS, __THROW, etc. stuff just the way that glibc has it.


> +#if defined _LIBC && defined __USE_LARGEFILE64
> +# ifdef __USE_GNU
> +struct stat64;
> +# endif

This isn't right, since glob.h can't depend on _LIBC (as mentioned above).
Please replace "defined _LIBC" with "!defined GLOB_PREFIX", everywhere
that you see _LIBC used (other than the changes already mentioned above).

> -# if defined __GNUC__ && __GNUC__ >= 2

This doesn't look right, since when installed as a part of glibc, the
header must be useable by GCC 1, or by non-GCC compilers.  Shouldn't
we leave this stuff in?  You might want to change it to

  # if defined __GNUC__ && __GNUC__ >= 2 && !defined GLOB_PREFIX

> -#if __USE_FILE_OFFSET64 && __GNUC__ < 2
> -# define glob glob64
> -# define globfree globfree64

Similarly, this code should be left in, with the first line changed to

  #if __USE_FILE_OFFSET64 && __GNUC__ < 2 && !defined GLOB_PREFIX

> -#if !defined __USE_FILE_OFFSET64 || __GNUC__ < 2
> +#if !defined _LIBC || !defined __USE_FILE_OFFSET64

Similarly, this should be:

  #if !defined __USE_FILE_OFFSET64 || __GNUC__ < 2 || defined GLOB_PREFIX