Re: [Findutils-patches] [PATCH] Include sys/stat.h in files where we use struct stat.

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to James Youngman on 6/23/2007 10:06 AM:
> 2007-06-23  James Youngman  <address@hidden>
> 
>       * build-aux/src-sniff.py: Detect uses of struct stat where the
>       header file was not included.

In general, looks okay (since it was mostly mechanical).  However, some
nits to think about:

>  
> -
> +#include <sys/stat.h>
>  #ifdef HAVE_FCNTL_H
>  #include <fcntl.h>

Not this patch, but gnulib provides fcntl.h, so that you can
unconditionally include it.

> @@ -106,8 +107,8 @@ int get_current_dirfd(void)
>  {
>    if (ftsoptions & FTS_CWDFD)
>      {
> -      assert(curr_fd != -1);
> -      assert( (AT_FDCWD == curr_fd) || (curr_fd >= 0) );
> +      assert (curr_fd != -1);
> +      assert ( (AT_FDCWD == curr_fd) || (curr_fd >= 0) );

That looks a bit odd to me, with extra spacing and parenthesis.  I think
GNU coding standards prefer:

assert (AT_FDCWD == curr_fd || curr_fd >= 0);

Paul Eggert would go further - he prefers < over > in all cases, and would
write that as:

assert (AT_FDCWD == curr_fd || 0 <= curr_fd);

although I'm not sold on that style yet.

> @@ -372,7 +373,7 @@ get_stat_Ytime(const struct stat *p,
>        *ret = get_stat_mtime(p);
>        return 1;
>      default:
> -      assert(0);
> +      assert (0);
>        abort();
>        abort();
>      }

There's some dead code.  As long as you are touching this hunk, you might
as well replace the double abort() with a single abort ().

> @@ -2743,8 +2744,8 @@ make_segment (struct segment **segment,
>      {
>      case KIND_PLAIN:         /* Plain text string, no % conversion. */
>      case KIND_STOP:          /* Terminate argument, no newline. */
> -      assert(0 == format_char);
> -      assert(0 == aux_format_char);
> +      assert (0 == format_char);
> +      assert (0 == aux_format_char);

Not introduced by your patch, but stylistically, I like to see '\0'
instead of 0 when dealing with char, just like I like to see NULL instead
of 0 when dealing with pointers.

> @@ -988,11 +982,16 @@ cost_table_comparison(const void *p1, const void *p2)
>  {
>    const struct pred_cost_lookup *pc1 = p1;
>    const struct pred_cost_lookup *pc2 = p2;
> -  
> -  
> +  void* p1 = (void*)pc1->fn;
> +  void* p2 = (void*)pc2->fn;

A comment here that you are intentionally casting away void might be helpful.

> +
> +  /* We use casts to void* for > comparison because not all C
> +   * compilers allow order comparison between functions.  For example,
> +   * that would fail on DEC Alpha OSF/1 with native cc.
> +   */
>    if (pc1->fn == pc2->fn)
>      return 0;
> -  else if (pc1->fn > pc2->fn)
> +  else if (p1 > p2)

You are using undefined C.  function pointers and void* are not guaranteed
to be compatible; and while in practice they are (think dlsym), it strikes
me as awkward to intentionally use undefined behavior.

> @@ -1151,7 +1150,7 @@ calculate_derived_rates(struct predicate *p)
>       else
>         {
>           /* only and, or and comma are BI_OP. */
> -         assert(0);
> +         assert (0);
>           rate = 0.0f;
>         }

Awkward dead code.  If you are asserting that something is unreachable,
you should abort rather than trying to reset the rate and go on.

> @@ -596,7 +596,7 @@ debug_stat (const char *file, struct stat *bufp)
>        return optionp_stat(file, bufp);
>      }
>    /*NOTREACHED*/
> -  assert(false);
> +  assert (false);
>    return -1;
>  }

Another instance.  And we probably ought to be consistent on whether we do
assert (0) or assert (false).

> @@ -679,7 +679,7 @@ main (int argc, char **argv)
>    /* Without SIZE_MAX (i.e. limits.h) this is probably 
>     * close to the best we can do.
>     */
> -  assert(sizeof(size_t) <= sizeof(unsigned long));
> +  assert (sizeof(size_t) <= sizeof(unsigned long));
>  #endif

This would be a candidate for the gnulib verify module, so you can make
this assertion at compile-time instead of runtime.

- --
Don't work too hard, make some time for fun as well!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGf7VS84KuGfSFAYARAuNDAJ4pCf5HWQjKaAI/53zvGuf+McohEwCgv/0t
ASiqWgCJxOyxKgW8YEluTJg=
=kOdZ
-----END PGP SIGNATURE-----