git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Jeff King <peff@peff.net>
To: SZEDER Gábor <szeder.dev@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	Ævar Arnfjörð Bjarmason <avarab@gmail.com>,
	Duy Nguyen <pclouds@gmail.com>, Luke Mewburn <luke@mewburn.net>,
	git@vger.kernel.org
Subject: Re: [PATCH v2 3/4] progress: clear previous progress update dynamically
Date: Tue, 2 Apr 2019 10:27:00 -0400
Message-ID: <20190402142700.GA10564@sigill.intra.peff.net> (raw)
In-Reply-To: <20190401141542.GJ32732@szeder.dev>

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

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

  reply index

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-25 10:38 [PATCH 0/5] Progress display fixes SZEDER Gábor
2019-03-25 10:38 ` [PATCH 1/5] progress: make display_progress() return void SZEDER Gábor
2019-03-25 10:38 ` [PATCH 2/5] progress: return early when in the background SZEDER Gábor
2019-03-25 11:08   ` Ævar Arnfjörð Bjarmason
2019-03-25 11:39     ` SZEDER Gábor
2019-03-26  6:28       ` Luke Mewburn
2019-03-26  5:38   ` Jeff King
2019-03-25 10:38 ` [PATCH 3/5] progress: assemble percentage and counters in a strbuf before printing SZEDER Gábor
2019-03-26  5:45   ` Jeff King
2019-03-27 10:24     ` SZEDER Gábor
2019-03-28  2:12       ` Jeff King
2019-03-25 10:38 ` [PATCH 4/5] progress: clear previous progress update dynamically SZEDER Gábor
2019-03-25 10:38 ` [PATCH 5/5] progress: break too long progress bar lines SZEDER Gábor
2019-03-25 11:02   ` Duy Nguyen
2019-03-25 11:12     ` SZEDER Gábor
2019-03-26  5:52 ` [PATCH 0/5] Progress display fixes Jeff King
2019-04-01 11:52 ` [PATCH v2 0/4] " SZEDER Gábor
2019-04-01 11:52   ` [PATCH v2 1/4] progress: make display_progress() return void SZEDER Gábor
2019-04-02  5:42     ` Eric Sunshine
2019-04-01 11:52   ` [PATCH v2 2/4] progress: assemble percentage and counters in a strbuf before printing SZEDER Gábor
2019-04-02  5:45     ` Eric Sunshine
2019-04-02  5:50       ` Eric Sunshine
2019-04-01 11:52   ` [PATCH v2 3/4] progress: clear previous progress update dynamically SZEDER Gábor
2019-04-01 13:30     ` Jeff King
2019-04-01 14:15       ` SZEDER Gábor
2019-04-02 14:27         ` Jeff King [this message]
2019-04-01 11:52   ` [PATCH v2 4/4] progress: break too long progress bar lines SZEDER Gábor
2019-04-05  0:45   ` [PATCH v3 0/4] Progress display fixes SZEDER Gábor
2019-04-05  0:45     ` [PATCH v3 1/4] progress: make display_progress() return void SZEDER Gábor
2019-04-05  0:45     ` [PATCH v3 2/4] progress: assemble percentage and counters in a strbuf before printing SZEDER Gábor
2019-04-05  0:45     ` [PATCH v3 3/4] progress: clear previous progress update dynamically SZEDER Gábor
2019-04-05  0:45     ` [PATCH v3 4/4] progress: break too long progress bar lines SZEDER Gábor
2019-04-05 22:21     ` [PATCH v3 0/4] Progress display fixes Jeff King
2019-04-12 19:45     ` [PATCH v4 " SZEDER Gábor
2019-04-12 19:45       ` [PATCH v4 1/4] progress: make display_progress() return void SZEDER Gábor
2019-04-12 19:45       ` [PATCH v4 2/4] progress: assemble percentage and counters in a strbuf before printing SZEDER Gábor
2019-04-12 19:45       ` [PATCH v4 3/4] progress: clear previous progress update dynamically SZEDER Gábor
2019-04-12 19:45       ` [PATCH v4 4/4] progress: break too long progress bar lines SZEDER Gábor

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190402142700.GA10564@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=luke@mewburn.net \
    --cc=pclouds@gmail.com \
    --cc=szeder.dev@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox