Hi Martin, On Tue, 30 Oct 2018, Martin Ågren wrote:
> It came up in review [1, 2] that this non-idiomatic loop is a bit tricky. > When we find a space, we set `len = i`, which gives us the answer we are > looking for, but which also breaks out of the loop. > > It turns out that this loop can confuse compilers as well. My copy of > gcc 7.3.0 realizes that we are essentially evaluating `(len + 1) < len` > and warns that the behavior is undefined if `len` is `INT_MAX`. (Because > the assignment `len = i` is guaranteed to decrease `len`, such undefined > behavior is not actually possible.) > > Rewrite the loop to a more idiomatic variant which doesn't muck with > `len` in the loop body. That should help compilers and human readers > figure out what is going on here. But do note that we need to update > `len` since it is not only used just after this loop (where we could > have used `i` directly), but also later in this function. > > While at it, reduce the scope of `i`. > > [1] > https://public-inbox.org/git/CAPig+cQbG2s-LrAo9+7C7=dXifbWFJ3SzuNa-QePHDk7egK=j...@mail.gmail.com/ > > [2] > https://public-inbox.org/git/capig+crju6nixpt2frdwz0x1hmgf1ojvzj3uk2qxege-s7i...@mail.gmail.com/ > > Helped-by: Eric Sunshine <sunsh...@sunshineco.com> > Signed-off-by: Martin Ågren <martin.ag...@gmail.com> > --- ACK. Thanks for cleaning up after me, Dscho > Thanks for the comments on v1. Based on them, I decided to go for > Eric's option 2, and to make the log message less technical in favor of > "compilers and humans alike can get this wrong". > > sequencer.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index 0c164d5f98..e7aa4d5020 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -2829,7 +2829,7 @@ static int do_reset(const char *name, int len, struct > replay_opts *opts) > struct tree_desc desc; > struct tree *tree; > struct unpack_trees_options unpack_tree_opts; > - int ret = 0, i; > + int ret = 0; > > if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) > return -1; > @@ -2849,10 +2849,13 @@ static int do_reset(const char *name, int len, struct > replay_opts *opts) > } > oidcpy(&oid, &opts->squash_onto); > } else { > + int i; > + > /* Determine the length of the label */ > for (i = 0; i < len; i++) > if (isspace(name[i])) > - len = i; > + break; > + len = i; > > strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name); > if (get_oid(ref_name.buf, &oid) && > -- > 2.19.1.593.gc670b1f876.dirty > >
