On Thu, Jan 02, 2025 at 07:58:24PM -0600, Justin Tobler wrote:
> 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@xxxxxx>
> > ---
> > 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
