Re: [PATCH 6/9] builtin/log: use `size_t` to track indices
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Jan 3, 2025, 06:43 UTC
- Message-ID
- <Z3eHAWnkrwyGqtki@pks.im>
- In-Reply-To
- <3b7ep2hex2vae56p2ba2kjeextjrjcsu5jrufr7hqewrnv3z45@ftrrcuvihxxs>
On Thu, Jan 02, 2025 at 07:58:24PM -0600, Justin Tobler wrote:
Show 32 quoted lines
> On 24/12/27 11:46AM, Patrick Steinhardt wrote:
> > Similar as with the preceding commit, adapt "builtin/log.c" so that it
> > tracks array indices via `size_t` instead of using signed integers. This
> > fixes a couple of -Wsign-compare warnings and prepares the code for
> > a similar refactoring of `repo_get_merge_bases_many()` in a subsequent
> > commit.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > builtin/log.c | 23 +++++++++++++----------
> > 1 file changed, 13 insertions(+), 10 deletions(-)
> >
> [snip]
> > if (show_progress)
> > progress = start_delayed_progress(_("Generating patches"), total);
> > - while (0 <= --nr) {
> > + for (i = 0; i < nr; i++) {
> > + size_t idx = nr - i - 1;
> > int shown;
> > - display_progress(progress, total - nr);
> > - commit = list[nr];
> > - rev.nr = total - nr + (start_number - 1);
> > +
> > + display_progress(progress, total - idx);
> > + commit = list[idx];
> > + rev.nr = total - idx + (start_number - 1);
>
> Along with updating array indices variables to use `size_t`, the loop
> structure here is also changed. Instead of iterating backwards from
> `nr`, the loop iterator increases and each iteration computes the index
> starting from the end. This is functionally the same behavior and it
> looks like it was done to improve readability.It wasn't really done to improve readability, but to retain correctness. Before this change we relied on `nr` being signed, and thus it could also have a negative value. Now that it's a `size_t` it would overflow eventually and that would make the loop condition a tautology. So I had to refactor the condition so that it doesn't rely on such an overflow anymore.
Patrick