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

[Paul Eggert]

Derek Price <address@hidden> writes:

>>First, what is "__ptr_t"?  Shouldn't it be replaced by "void *"
>>uniformly?
>
> This is one of the items I didn't touch since it didn't raise any
> compiler warnings here.  It looks like it is mostly being used as a void
> *, though I'm not sure why since there are a few direct void * decls. 

My guess is that __ptr_t is a glibc thing, and is present to allow
ports to pre-C89 hosts that lacked support for void *.  We no longer
worry about such hosts, so we can simply use void *.

> It looks like there are a few unnecessary typecasts

Possibly they're needed for people who use C++ compilers to
compile C code?

> It's okay to assume C89 or better for glibc?  I knew it was correct for
> GNULIB but I was unsure about glibc.

Yes, it's OK.

> Otherwise, my correction is as you specified, but if we can assume C89
> in glob.c, why can't we assume C89 here and just #include <stddef.h>?

glob.h is not supposed to infringe on the user namespace when it's
part of glibc.  However, it's OK (and to some extent unavoidable) for
glob.h to do so when it's part of user code.

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

I'd omit this change; it's not necessary and it just complicates
the job of evaluating the patch.

>  # ifdef HAVE_VMSDIR_H
> +   /* Should this be imported from glibc?  */
>  #  include "vmsdir.h"

glibc doesn't have a vmsdir.h.  This area should be fixed up but this
is a separate matter; I'd omit this change for now.

> +# define CONVERT_D_TYPE(d64, d32)

This macro isn't used anywhere and can be removed.

> +# define MUST_BE(d, t)       ((d)->d_type == (t))
> +# define MAY_BE_LINK(d)      (MUST_BE((d), DT_UNKNOWN) || MUST_BE((d), 
> DT_LNK))
> +# define MAY_BE_DIR(d)       (MUST_BE((d), DT_DIR) || MAY_BE_LINK (d))

This is a bit hard to follow, I'm afraid, and the names are
contributing to the confusion.  We should use "MIGHT" instead of
"MAY", to avoid the classic "may" ambiguity.  How about if we rename
MUST_BE to DIRENT_MUST_BE, MAY_BE_LINK to DIRENT_MIGHT_BE_LINK, and
MAY_BE_DIR to DIRENT_MIGHT_BE_DIR?  We also refrain from having the
MAY_BE macros call MUST_BE.  Something like this:

/* True if the directory entry D must be of type T.  */  
# define DIRENT_MUST_BE(d, t) ((d)->d_type == (t))

/* True if the directory entry D might be a symbolic link.  */
# define DIRENT_MIGHT_BE_SYMLINK(d) \
    ((d)->d_type == DT_LNK || (d)->d_type == DT_UNKNOWN)

/* True if the directory entry D might be a directory.  */
# define DIRENT_MIGHT_BE_DIR(d) \
    ((d)->d_type == DT_DIR || DIRENT_MIGHT_BE_SYMLINK (d))

> -#if _LIBC
> +#if _LIBC || HAVE_FUNCTION_DIRENT64
>  # define HAVE_DIRENT64       1

I don't see why this change is needed.  The .m4 file sets
HAVE_DIRENT64.

> +/* GNULIB includes that shouldn't be used when copiling LIBC.  */

I'd remove this comment; it's obvious.

> +/* Correct versions of these headers all come from GNULIB when missing or
> + * broken.
> + */

Likewise.  Also, please use GNU style for comments (no leading-*).

  #ifdef HAVE_FUNCTION_STAT64
  /* Assume the struct stat64 type.  */
  # define HAVE_STAT64  1
  #endif

The m4 file defines HAVE_STAT64, so this entire code block should be
removed.

>  #ifndef HAVE_STAT64
> +  /* Use stat() instead.  */
>  # define __stat64(fname, buf) __stat (fname, buf)

I'd omit this change; the comment is obvious.

> +# define stat64 stat

I don't see why this #define is needed; doesn't the code use __stat64
uniformly?

> +#elif !_LIBC

Generally the code uses "defined _LIBC" rather than "_LIBC".  It's not
universally true, but I'd stick to the strong-majority style when adding
new code.

> +  /* GNULIB needs the usual versions.  */
> +# define __stat64 stat64

First, lose the comment (it's obvious); second, all the #define's of
__stat64 should be kept near each other.

> +  /* GNULIB needs the usual versions.  */

Again, the comment isn't needed.

> +# define __stat stat
> +# define __alloca alloca
> +# define __readdir readdir
> +# define __readdir64 readdir64

Perhaps these should be put next to the __stat64 define?

> -#include <glob.h>

The Gnulib tradition is to include the interface file (here, <glob.h>)
first, immediately after including <config.h>.  Could you please do
that here, too?

> -#ifdef HAVE_GETLOGIN_R
> +#ifdef HAVE_FUNCTION_GETLOGIN_R

This change shouldn't be needed.

> -
> +
> +
> +
>  static const char *next_brace_sub (const char *begin, int flags) __THROW;

This change shouldn't be needed.

> -#   if defined HAVE_GETLOGIN_R || defined _LIBC
> +#   if defined HAVE_FUNCTION_GETLOGIN_R || defined _LIBC

This change shouldn't be needed.

> -#   if defined HAVE_GETPWNAM_R || defined _LIBC
> +#   if defined HAVE_FUNCTION_GETPWNAM_R || defined _LIBC

Nor this.

> -#  if defined HAVE_GETPWNAM_R || defined _LIBC
> +#  if defined HAVE_FUNCTION_GETPWNAM_R || defined _LIBC

Nor this.

> -#endif
> +#endif /* !defined _LIBC || !defined NO_GLOB_PATTERN_P */

Nor this.

> -static int
> -link_exists_p (const char *dir, size_t dirlen, const char *fname,
> -            glob_t *pglob, int flags)
> +static bool
> +is_dir_p (const char *dir, size_t dirlen, const char *fname,
> +       glob_t *pglob, int flags)

The bool->int is fine, but why change the name?

> -  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);

Let's keep the original extra set of parentheses.  It's not an issue
of return x; versus return (x); it's the usual GNU style that
expressions that cover more than one line are parenthesized.

We need to use this style in the definition of isdir, as well.

> +
>  /* Like `glob', but PATTERN is a final pathname component,

This change (just a blank line) isn't needed.

> +                   /* Note that isdir will often be incorrectly set to false
> +                    * when not in GLOB_ONLYDIR || GLOB_MARK mode, but we
> +                    * don't care.  It won't be used and we save the
> +                    * expensive call to stat().

Use the GNU style (no leading "*") for comment.  Remove the "Note
that"; it's redundant.  'stat()' -> '"stat"'.

> +                   bool isdir = MUST_BE (d, DT_DIR)
> +                                || ((flags & GLOB_ONLYDIR)
> +                                    && MAY_BE_LINK (d)
> +                                    && is_dir_p (directory, dirlen, name,
> +                                                 pglob, flags))
> +                                || ((flags & GLOB_MARK)
> +                                    && is_dir_p (directory, dirlen, name,
> +                                                 pglob, flags));

Looks you can get by with just one call to link_exists_p.  How about
this?

int need_link_test =
  (GLOB_MARK | (DIRENT_MIGHT_BE_LINK (d) ? GLOB_ONLYDIR : 0));
bool isdir =
  (DIRENT_MUST_BE (d, DT_DIR)
   || ((flags & need_link_test)
       && link_exists_p (directory, dirlen, name, pglob, flags)));



> +                   /* In GLOB_ONLYDIR mode, skip non-dirs.  */
> +                   if ((flags & GLOB_ONLYDIR) && !isdir)
> +                       continue;
> +
>                       {

The indenting looks fishy here.

I've run out of time; perhaps another round?