Re: [Bug-gnulib] full-write.c / full-read.c consolidation

[Jim Meyering]

Bruno Haible <address@hidden> wrote:
> Jim Meyering writes:
>> Any objection to this?
>>
>>      * full-write.c: Rework so that it may serve to define full_read, too.
>>      * full-read.c: Simply #define FULL_READ and include full-write.c.
>>
>> With the above change, full-read.c is just two lines (modulo copyright):
>
> Nothing against unification of duplicated code - where it is
> duplicated and not only similar. But here, when 50% of the resulting
> code is #ifdef'ed, my gut feeling is that we are going too far. The
> #ifdef spaghetti is harder to maintain than the two earlier functions.

Hi Bruno,

I didn't like the in-function #ifdef either, so factored that out
after sending that message but before checking in to gnulib.
The new code is cleaner.  Here it is:

  size_t
  full_rw (int fd, const void *buf, size_t count)
  {
    size_t total = 0;
    const char *ptr = buf;

    while (count > 0)
      {
        size_t n_rw = safe_rw (fd, ptr, count);
        if (n_rw == (size_t) -1)
          break;
        if (n_rw == 0)
          {
            errno = ZERO_BYTE_TRANSFER_ERRNO;
            break;
          }
        total += n_rw;
        ptr += n_rw;
        count -= n_rw;
      }

    return total;
  }

I don't like all the #ifdef'd includes.
They're just for prototypes after all, so it shouldn't
hurt to include e.g. safe-read.h from safe-write.c.

>> # undef const
>> # define const /* empty */

Thanks for catching that.
I've just moved the #include `up'.

> I think it would be safer to do this after including <errno.h>, not
> before. You never know what <errno.h> contains.
>
>>   const char *ptr = buf;
>>
>>   while (count > 0)
>
> Any reason why you undid the "really nothing to do if count == 0"
> optimization that I had put in?

I found that it made the code much less readable, and for
probably-imperceptible gain.  Besides, who cares how efficient the
code is when it's asked to do nothing?  Of course, if you know of some
application where that single unnecessary store (*ptr = buf;) actually
makes a difference, then I'll be happy to reconsider.

Jim