git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
MÅMartin Ågren <martin.agren@gmail.com>
Date
Oct 28, 2018, 20:37 UTC
Message-ID
<CAN0heSpiVUj1LK9RFkJ5VWGVZs=mheYwYjut3YXzMfVYhtU+8A@mail.gmail.com>
In-Reply-To
<CAPig+cR28OBA0rwy=X6XU0cjstAguvUiLU9Zxjvg2X7=UsB5qw@mail.gmail.com>
On Sun, 28 Oct 2018 at 20:01, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 13 quoted lines
>
> 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/

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

Show 23 quoted lines
> >                 /* 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
Previous: Eric SunshineNext: Junio C Hamano
Message 3 of 8 in “sequencer: clarify intention to break out of loop”
  1. sequencer: clarify intention to break out of loopMartin Ågren, Oct 28, 2018
  2. Eric SunshineOct 28, 2018
  3. Martin ÅgrenOct 28, 2018
  4. Junio C HamanoOct 29, 2018
  5. sequencer: break out of loop explicitlyMartin Ågren, Oct 30, 2018
  6. Johannes SchindelinOct 31, 2018
  7. Eric SunshineOct 31, 2018
  8. Martin ÅgrenOct 31, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.