Re: [PATCH] compat/win32: correct for incorrect compiler warning

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 19, 2022, 20:13 UTC
Message-ID: <xmqqk088amj4.fsf@gitster.g>
In-Reply-To: <pull.1294.git.1658256354725.gitgitgadget@gmail.com>

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 23 quoted lines

> diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
> index 1f8d8934cc9..0af18d88825 100644
> --- a/compat/win32/syslog.c
> +++ b/compat/win32/syslog.c
> @@ -44,6 +44,7 @@ void syslog(int priority, const char *fmt, ...)
>  
>  	while ((pos = strstr(str, "%1")) != NULL) {
>  		size_t offset = pos - str;
> +		char *new_pos;
>  		char *oldstr = str;
>  		str = realloc(str, st_add(++str_len, 1));
>  		if (!str) {
> @@ -51,9 +52,9 @@ void syslog(int priority, const char *fmt, ...)
>  			warning_errno("realloc failed");
>  			return;
>  		}
> -		pos = str + offset;
> -		memmove(pos + 2, pos + 1, strlen(pos));
> -		pos[1] = ' ';
> +		new_pos = str + offset;
> +		memmove(new_pos + 2, new_pos + 1, strlen(new_pos));
> +		new_pos[1] = ' ';
>  	}

Heh, after this sequence

	pos = strstr(str, ...);
	str = realloc(str, ...);
	pos = str + something;

the compiler warns about use of pos[] as if resetting of pos based on the new value of str[] did not even exist?

Discovering a working work-around must have been a pain.

If I were writing this, I'd probably

 * avoid re-scanning "str" from the beginning in every iteration;

 * possibly avoid reallocating for %1 by first scanning and counting
   the problematic "%1";

 * also possibly give a deeper thought to see if "%1" -> "% 1" is
   necessary.  We are munging the original string anyway---in the
   output that has "% 1", you cannot tell if the original was "%1"
   that was munged by this code, or it was "% 1" from the beginning.
   In other words, we are already willing to lose information.
   Perhaps we can find a different replacement sequence that is
   still 2-byte-wide and that is unlikely to appear in the original,
   eliminating the need for reallocation altogether?

But none of that is a problem with this patch.

Will queue.  Thanks.