[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Findutils-patches] [PATCH] Include sys/stat.h in files where we use
From: |
Eric Blake |
Subject: |
Re: [Findutils-patches] [PATCH] Include sys/stat.h in files where we use struct stat. |
Date: |
Mon, 25 Jun 2007 06:30:10 -0600 |
User-agent: |
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.12) Gecko/20070509 Thunderbird/1.5.0.12 Mnenhy/0.7.5.666 |
-----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-----
- [Findutils-patches] [PATCH] Include sys/stat.h in files where we use struct stat., James Youngman, 2007/06/24
- Re: [Findutils-patches] [PATCH] Include sys/stat.h in files where we use struct stat.,
Eric Blake <=
- Re: [Findutils-patches] [PATCH] Include sys/stat.h in files where we use struct stat., James Youngman, 2007/06/26
- Re: [Findutils-patches] [PATCH] Include sys/stat.h in files where we use struct stat., James Youngman, 2007/06/26
- Re: [Findutils-patches] [PATCH] Include sys/stat.h in files where we use struct stat., Eric Blake, 2007/06/26
- Re: [Findutils-patches] [PATCH] Include sys/stat.h in files where we use struct stat., James Youngman, 2007/06/26
- Re: [Findutils-patches] [PATCH] Include sys/stat.h in files where we use struct stat., Bob Proulx, 2007/06/26
- Re: [Findutils-patches] [PATCH] Include sys/stat.h in files where we use struct stat., Eric Blake-1, 2007/06/26