bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Paul Eggert
Subject: Re: [bug-gnulib] New GNULIB glob module?
Date: Wed, 11 May 2005 16:47:56 -0700
User-agent: Gnus/5.1006 (Gnus v5.10.6) Emacs/21.4 (gnu/linux)

Derek Price <address@hidden> writes:

> I needed a portable version of glob for CVS, so I imported the glob
> module from glibc 2.3.5 into a GNULIB format.

Wow!  Nice project.

> Mostly I kept the glibc version intact, though I managed to simplify
> some portability cruft immensely by replacing huge chunks of code
> with #includes of GNULIB modules.

Ideally we should write code that can be folded back into glibc,
identical to the gnulib version.  That should be doable here, but some
more hacking is needed.

First, what is "__ptr_t"?  Shouldn't it be replaced by "void *"
uniformly?

> -/* AIX requires this to be the first thing in the file.  */
> -#if defined _AIX && !defined __GNUC__
> - #pragma alloca
> -#endif

This change is good, since glibc doesn't need that code and gnulib
doesn't either (any more).

> -#if defined HAVE_LIMITS_H || defined __GNU_LIBRARY__
> -# include <limits.h>
> -#endif
> +#include <limits.h>

This sort of change is good, since gnulib assumes C89 or better now.

> -#define GLOB_INTERFACE_VERSION 1
> -#if !defined _LIBC && defined __GNU_LIBRARY__ && __GNU_LIBRARY__ > 1
> -# include <gnu-versions.h>
> -# if _GNU_GLOB_INTERFACE_VERSION == GLOB_INTERFACE_VERSION
> -#  define ELIDE_CODE
> -# endif
> -#endif

This removal looks reasonable, except shouldn't glob.m4 check that
_GNU_GLOB_INTERFACE_VERSION is 1?  It can do that as a compile-time
check.

> -#if defined HAVE_UNISTD_H || defined _LIBC
> +#if HAVE_HEADER_UNISTD_H || _LIBC

This sort of change doesn't look right.  Why not leave the code alone?
Nothing ever defines HAVE_HEADER_UNISTD_H.  There are several other
instances of this sort of thing, e.g., HAVE_HEADER_SYS_NDIR_H,
HAVE_DIRENT64.

> -#if _LIBC
> -# define HAVE_DIRENT64       1
> +#if defined _DIRENT_HAVE_D_TYPE && !defined HAVE_DIRENT_D_TYPE
> +# define HAVE_DIRENT_D_TYPE  1
>  #endif

This should use HAVE_STRUCT_DIRENT_D_TYPE, from d-type.m4.  Also,
don't see why you need to change the spelling of HAVE_D_TYPE.  Please
try to leave the identifiers the same, so that the glibc maintainers
have an easier time of following the patch.

> -#ifdef _LIBC
> -# include <alloca.h>
> -# undef strdup
> -# define strdup(str) __strdup (str)
> -# define sysconf(id) __sysconf (id)
> -# define closedir(dir) __closedir (dir)
> -# define opendir(name) __opendir (name)
> -# define readdir(str) __readdir64 (str)
> -# define getpwnam_r(name, bufp, buf, len, res) \
> -   __getpwnam_r (name, bufp, buf, len, res)
> -# ifndef __stat64
> -#  define __stat64(fname, buf) __xstat64 (_STAT_VER, fname, buf)
> -# endif
> -# define HAVE_STAT64 1
> -#endif

Please leave this sort of code in, so that the source code can be
identical in libc.

> +#include "mempcpy.h"
> +#include "stat-macros.h"
> +#include "strdup.h"

These includes need to be protected by #ifndef _LIBC.

> +int glob_pattern_p (const char *pattern, int quote);

I don't see why this declaration is needed in glob.c.
Isn't it in glob_.h?

> -       if (__glob_pattern_p (drive_spec, !(flags & GLOB_NOESCAPE)))
> +       if (glob_pattern_p (drive_spec, !(flags & GLOB_NOESCAPE)))

I'd leave the code alone here, to minimize the libc changes.
You can put a "#define __glob_pattern_p glob_pattern_p" inside
Similarly for __stat64, etc.

> -  if (flags & GLOB_MARK)
> -    {
> -      /* Append slashes to directory names.  */
> -      size_t i;

Why remove this feature?  Is there any harm to leaving it alone?

>  int
> -__glob_pattern_p (pattern, quote)
> -     const char *pattern;
> -     int quote;
> +glob_pattern_p (const char *pattern, int quote)

This sort of change is good.

> -# ifdef _LIBC
> -weak_alias (__glob_pattern_p, glob_pattern_p)
> -# endif

We can leave the code alone here; it doesn't hurt.

>    struct stat st;
> +# ifdef HAVE_FUNCTION_STAT64
>    struct stat64 st64;
> +# endif

This sort of thing is awkward.  How about if you remove the
"# define st64 st" line?  Then you can omit this sort of change.

> -  return (((flags & GLOB_ALTDIRFUNC)
> -        ? (*pglob->gl_stat) (fullname, &st)
> -        : __stat64 (fullname, &st64)) == 0);
> +  return flags & GLOB_ALTDIRFUNC
> +      ? (*pglob->gl_stat) (fullname, &st) == 0 && S_ISDIR (st.st_mode)
> +      : stat64 (fullname, &st64) == 0 && S_ISDIR (st64.st_mode);

Please stick with the original indenting style and names.

Why did you insert the && S_ISDIR here?  Is this a bug fix?  It's not
clear from the patch.  (Also, you need a ChangeLog entry suitable for
sending this patch to the glibc folks.)

> +#ifdef HAVE_DIRENT_D_TYPE
> +# define MAY_BE_LINK(d) (d->d_type == DT_UNKNOWN || d->d_type == DT_LNK)
> +# define MAY_BE_DIR(d)       (d->d_type == DT_DIR || MAY_BE_LINK (d))

Parenthesize (d) and move these definitions to an early part of the
program, and put in an else-part, e.g.:

#if HAVE_D_TYPE
# define MAY_BE_LINK(d) ((d)->d_type == DT_UNKNOWN || (d)->d_type == DT_LNK)
# define MAY_BE_DIR(d)  ((d)->d_type == DT_DIR || MAY_BE_LINK (d))
# define MUST_BE(d, t)  ((d)->d_type == (t))
#else
# define MAY_BE_LINK(d) true
# define MAY_BE_DIR(d)  true
# define MUST_BE(d, t)  false
#endif


> +
>                 /* If we shall match only directories use the information
> -                  provided by the dirent call if possible.  */
> -               if ((flags & GLOB_ONLYDIR)
> -                   && d->d_type != DT_UNKNOWN
> -                   && d->d_type != DT_DIR
> -                   && d->d_type != DT_LNK)
> +                  provided by the dirent call if possible.
> +                *
> +                * This will still be caught via stat() later, but stat()
> +                * and fnmatch() are expensive and this saves the call to
> +                * fnmatch() when this information is already available from
> +                * the dirent structure.
> +                */
> +               if (flags & GLOB_ONLYDIR && !MAY_BE_DIR (d))

Omit the comment change here; it's obvious.  Also, keep the
parentheses around (flags & GLOB_ONLYDIR) (both here, and in other
places).


> +#ifdef HAVE_DIRENT_D_TYPE
> +                                d->d_type == DT_DIR
> +#endif

Change this to MUST_BE (d, DT_DIR), and omit the #ifdef.

> +#ifdef HAVE_DIRENT_D_TYPE
> +                                    && MAY_BE_LINK (d)
> +#endif

This can be plain "&& MAY_BE_LINK (d)" without the ifdef.

> -#include <sys/cdefs.h>
> -
> -__BEGIN_DECLS
> -
>  /* We need `size_t' for the following definitions.  */
> -#ifndef __size_t
> -# if defined __GNUC__ && __GNUC__ >= 2
> -typedef __SIZE_TYPE__ __size_t;
> -#  ifdef __USE_XOPEN
> -typedef __SIZE_TYPE__ size_t;
> -#  endif
> -# else
> -#  include <stddef.h>
> -#  ifndef __size_t
> -#   define __size_t size_t
> -#  endif
> -# endif
> -#else
> -/* The GNU CC stddef.h version defines __size_t as empty.  We need a real
> -   definition.  */
> -# undef __size_t
> -# define __size_t size_t
> -#endif
> +#include <stddef.h>

This should be something like "#ifdef _LIBC [same stuff as before]
#else #include <stddef.h> #ifndef __size_t #define __size_t size_t
#endif #endif".  Then you can leave the existing __size_t's alone.

> -#if __USE_FILE_OFFSET64 && __GNUC__ < 2
> +#if 0 && __USE_FILE_OFFSET64 && __GNUC__ < 2

I don't get why we need the "0 &&" here?  Anyway, for the _LIBC case it
should be left alone.

Whew!  I've run out of time.  Can you please look into these comments?
We can do the cycle again, after you've had time to digest them.
Thanks again.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]