Re: [PATCH v2] sequencer: break out of loop explicitly

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 2547 bytes --]

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=jg@mail.gmail.com/
> 
> [2] https://public-inbox.org/git/CAPig+cRjU6niXpT2FrDWZ0x1HmGf1ojVZj3uk2qXEGe-S7i_HQ@mail.gmail.com/
> 
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Martin Ågren <martin.agren@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
> 
>