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/26/2007 2:01 AM:
> On 6/25/07, Eric Blake <address@hidden> wrote:
>> 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);
> 
> I have no objection to that style particularly, but I prefer people
> who see that the assertion failed not to have to check their mental
> operator precedence list.

Reasonable enough to keep the inner parenthesis when they are
user-visible.  However, there's still the question of whether
 assert ( (cond1) || (cond2) );
or
 assert ((cond1) || (cond2));

looks nicer, but a quick check shows that, for gcc 3.4.4 at least, the
leading and trailing space of the former are not stringized.

> 
> I agree with you on the issue of NULL, but I am in firm diagreement on
> the issue of '\0'.   As I am sure you know, in C the type of '\0' is
> int, not char.   Hence '\0' and 0 are of the same type and have the
> same value.  Choice between them is essentially stylistic.
> 
> But I have found in the past that typos are a frequent source of bugs.
>  The best-known case of this is the accidental change of '==' to '='.
>  However, in this case a typo can change '\0' to '0', introducing a
> bug.  For this reason I prefer to use 0 rather than '\0' because the
> former isn't vulnerable to bugs introduced as typos.

You provide a nice argument.  Maybe it's worth documenting that in the
findutils maintainers note, since the GNU Coding Standards are silent on
preference between 0 and '\0'.

>> A comment here that you are intentionally casting away void might be
>> helpful.
> 
> If you mean casting away const, that was a bug, thanks for finding it,
> but it was fixed in a later patch.

Yes, that's what I meant.

> 
> I have an idea that it might be standard-conforming to assign the
> function pointer values to a union member and use memcmp().   What do
> you think?

As in the following?  Yes, I think this is safer:

union {
  pred_cost_lookup* fn;
  char mem[sizeof (pred_cost_lookup*)];
} u1, u2;
u1.fn = p1;
u2.fn = p2;
return memcmp (u1.mem, u2.mem, sizeof (pred_cost_lookup*));

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

iD8DBQFGgQvB84KuGfSFAYARAk4YAKC6Ye/Pf8g1CGR5RleHVic6Xs5W0ACg1/EB
6rdOBrnZiLHLNqT1+UFumfc=
=UAXW
-----END PGP SIGNATURE-----