Web lists-archives.com

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




On Sun, 28 Oct 2018 at 20:01, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>
> On Sun, Oct 28, 2018 at 11:32 AM Martin Ågren <martin.agren@xxxxxxxxx> 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@xxxxxxxxxxxxxx/
> [2]: https://public-inbox.org/git/CAPig+cRjU6niXpT2FrDWZ0x1HmGf1ojVZj3uk2qXEGe-S7i_HQ@xxxxxxxxxxxxxx/

Hmm, I should have been able to dig those up myself. Thanks for the
pointers.

> >                 /* 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);

This second one is more true to the original in that it updates `len` to
the new, shorter length. Which actually seems to be needed -- towards
the very end of the function, `len` is used, so the first of these
suggestions would change the behavior.

Thanks a lot for a review. I'll wait for any additional comments and
will try a v2 with your second suggestion.

Martin