Re: [PATCH v2 3/4] progress: clear previous progress update dynamically

From: Jeff King <peff@peff.net>
Date: Apr 2, 2019, 14:27 UTC
Message-ID: <20190402142700.GA10564@sigill.intra.peff.net>
In-Reply-To: <20190401141542.GJ32732@szeder.dev>

On Mon, Apr 01, 2019 at 04:15:42PM +0200, SZEDER Gábor wrote:

Show 30 quoted lines

> > I don't think it could matter here, as these are meant to be smallish
> > strings, but I think we should get into the habit of using size_t
> > consistently to hold string lengths.
> > 
> > It makes auditing for integer overflow problems much simpler (this is on
> > my mind as I happen to be tracing some bugs around this the past few
> > days).
> > 
> > (There are a few instances in the next patch, too. Other than this nit,
> > though, your series looks good to me).
> 
> I started with using size_t, but then switched to int, because:
> 
>   - After a bit of arithmetic I had to compare to term_columns()'s int
>     return value anyway (in the next patch).
> 
>   - The second hunk in this patch adds these lines:
> 
>         int clear_len = counters_sb->len < last_count_len ?
>                         last_count_len - counters_sb->len : 0;
>         fprintf(stderr, "%s: %s%-*s", progress->title,
>                 counters_sb->buf, clear_len, eol);
> 
>     Here 'clear_len' has to be int, because the printf() format "%-*s"
>     expects an int, and otherwise -Werror=format= errors ensue.  OK,
>     it could be size_t, but then it must be casted to an int upon
>     passing it to fprintf(), and after the next patch there will be
>     three such calls.
> 
> I could resend using size_t.  Should I resend using size_t? :)

IMHO it's better to keep it as a size_t for as long as possible, and then cast when we pass to printf, for a few reasons:

  1. The cast is made explicitly, so it calls attention to it.

  2. We know that a cast to int there can at worst produce truncated
     output, and not lead to any kind of memory error. (And if we really
     care about that, it's easy to convert it to an fwrite() at that
     point, though I would not bother in this case).

I think the comparison to "cols" is OK, because we are just checking whether cols is smaller than us. If we assume that size_t is at least as big as an int (which I think is a reasonable assumption to make, and certainly holds true for all platforms I know) then there's no possibility of logic errors.

I wouldn't even bother with a cast there. Probably -Wsign-compare would complain, but we are pretty far from enabling that. And I think the right solution is for term_columns() to return an unsigned, anyway. ;)

I admit all of this is academic enough that I can live with it either way (there are definitely places where it is _not_ academic, so I am mostly just trying to encourage a general style).

-Peff