Re: [PATCH] sequencer: clarify intention to break out of loop

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 28, 2018, 19:01 UTC
Message-ID: <CAPig+cR28OBA0rwy=X6XU0cjstAguvUiLU9Zxjvg2X7=UsB5qw@mail.gmail.com>
In-Reply-To: <20181028153145.25734-1-martin.agren@gmail.com>

On Sun, Oct 28, 2018 at 11:32 AM Martin Ågren <martin.agren@gmail.com> wrote:

> [...]
> Let's be explicit about breaking out of the loop. This helps the
> compiler grok our intention. As a bonus, it might make it (even) more
> obvious to human readers that the loop stops at the first space.

This did come up in review[1,2]. I had a hard time understanding the loop because it looked idiomatic but wasn't (and we have preconceived notions about behavior of things which look idiomatic).

[1]: https://public-inbox.org/git/CAPig+cQbG2s-LrAo9+7C7=dXifbWFJ3SzuNa-QePHDk7egK=jg@mail.gmail.com/ [2]: https://public-inbox.org/git/CAPig+cRjU6niXpT2FrDWZ0x1HmGf1ojVZj3uk2qXEGe-S7i_HQ@mail.gmail.com/

Show 12 quoted lines

> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -2849,10 +2849,14 @@ static int do_reset(const char *name, int len, struct replay_opts *opts)
>                 /* Determine the length of the label */
> +               for (i = 0; i < len; i++) {
> +                       if (isspace(name[i])) {
>                                 len = i;
> +                               break;
> +                       }
> +               }
>                 strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);

At least for me, this would be more idiomatic if it was coded like this:

    for (i = 0; i < len; i++)
        if (isspace(name[i]))
            break;
    strbuf_addf(..., i, name);

or, perhaps (less concise):

    for (i = 0; i < len; i++)
        if (isspace(name[i]))
            break;
    len = i;
    strbuf_addf(..., len, name);