findutils-patches
[Top][All Lists]
Advanced

[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: James Youngman
Subject: Re: [Findutils-patches] [PATCH] Include sys/stat.h in files where we use struct stat.
Date: Tue, 26 Jun 2007 09:01:40 +0100

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.


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 ().

Done.


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


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.




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

If you mean casting away const, that was a bug, thanks for finding it,
but it was fixed in a later patch.

> +  /* 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.

I fully agree.  The problem is that pc1->fn and pc2->fn are pointers
to functions.  Since these are not pointers to object types, the < and
operators are undefined for them (specifically, the "one of the
following shall hold constraint in section 6.3.8 doesn't include
pointers to functions).

So, we were using undefined C before, just like now.  But the
undefined C we were using before didn't work.  The undefined C we're
using now will probably work more widely.

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?

>         {
>           /* 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.

I have removed the dead assignment and replaced it with a call to
abort.  Thanks.


> @@ -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).

Removed dead code.  Inserted abort().  Changed assert (false) to
assert (0), which turned out to be the more common of the two
alternatives.


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

Thanks; I raised a bug to remind me to do that.

James.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]