From: Jeff King <peff@peff.net>
Date: Fri, 01 Jan 2016 08:42:06 GMT
Subject: Re: [PATCH 02/14] log: refactor add_header to drop some magic numbers
Message-ID: <20160101084206.GB20715@sigill.intra.peff.net>
In-Reply-To: <CAPig+cRgK5Ey8WNDLLOnAhR+xh6NEHk-hGhGccj4SkZO-RV_-Q@mail.gmail.com>

On Thu, Dec 31, 2015 at 01:21:42AM -0500, Eric Sunshine wrote:

> > -       item->string[len] = '\0';
> > +       len = strlen(item->string);
> > +       while (len && item->string[len - 1] == '\n')
> > +               item->string[--len] = '\0';
> 
> Not a strong objection, but this implementation may make the reader
> wonder why NUL needs to be assigned to all "stripped" characters. I'd
> have expected to see:
> 
>     len = strlen(item->string);
>     while (len && item->string[len - 1] == '\n')
>         len--;
>     item->string[len] = '\0';
> 
> which indicates clearly that this is a simple truncation rather than
> some odd NUL-fill operation, and is slightly more easy to reason about
> since it doesn't involve a pre-decrement as an array subscript.

Hmm. I consider the "write NULs backward" strategy to be pretty
idiomatic (you can find several similar ones grepping for `\[--` in the
git codebase). But that may just be me (I didn't look, but it's possible
I wrote the other ones, too :) ).

I don't have a strong preference, though. What you've written is quite
readable.

-Peff