Derek Price <[EMAIL PROTECTED]> 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. _______________________________________________ Bug-cvs mailing list Bug-cvs@gnu.org http://lists.gnu.org/mailman/listinfo/bug-cvs