Re: [PATCH] revision: use C99 declaration of variable in for() loop

[Phillip Wood <phillip.wood123@gmail.com>]

On 18/11/2021 07:09, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> I like the idea of using a specific test balloon for the features that
>> we want to use but wont this one break the build for anyone doing
>> 'make DEVELOPER=1' because -Wdeclaration-after-statement will error
>> out.
> 
> I think you are missing '?' at the end of the sentence,

sorry yes I am

> but the
> answer is "no, at least not for me".
> 
>      # pardon my "make" wrapper; it is to pass DEVELOPER=1 etc. to
>      # the underlying "make" command.
>      $ Meta/Make V=1 revision.o
>      cc -o revision.o -c -MF ./.depend/revision.o.d -MQ revision.o -MMD -MP  -Werror -Wall -pedantic -Wpedantic -Wdeclaration-after-statement -Wformat-security -Wold-style-definition -Woverflow -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla -fno-common -Wextra -Wmissing-prototypes -Wno-empty-body -Wno-missing-field-initializers -Wno-sign-compare -Wno-unused-parameter  -g -O2 -Wall -I. -DHAVE_SYSINFO -DGIT_HOST_CPU="\"x86_64\"" -DUSE_LIBPCRE2 -DHAVE_ALLOCA_H  -DUSE_CURL_FOR_IMAP_SEND -DSUPPORTS_SIMPLE_IPC -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"cache.h\"" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DSHA256_BLK  -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_SYNC_FILE_RANGE -DHAVE_GETDELIM '-DPROCFS_EXECUTABLE_PATH="/proc/self/exe"' -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -DSHELL_PATH='"/bin/sh"' -DPAGER_ENV='"LESS=FRX LV=-c"'  revision.c
>      $ cc --version
>      cc (Debian 10.3.0-11) 10.3.0
>      Copyright (C) 2020 Free Software Foundation, Inc.
>      This is free software; see the source for copying conditions.  There is NO
>      warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> 
> 
> It would be quite sad if we had to allow decl-after-stmt, only to
> allow
> 
> 	stmt;
> 	for (type var = init; ...; ...) {
> 		...;
> 	}
> 
> because it should merely be a short-hand for
> 
> 	stmt;
> 	{
> 	    type var;
> 	    for (var = init; ...; ...) {
> 		...;
> 	    }
> 	}
> 
> that does not need to allow decl-after-stmt.
> 
> Different compilers may behave differently, so it might be an issue
> for somebody else, but I am hoping any reasonable compiler would
> behave sensibly.
> 
> Thanks for raising a potential issue, as others can try it out in
> their environment and see if their compilers behave well.

Oh it seems I misunderstood exactly what decl-after-stmt does, thinking 
about it your explanation makes sense. I guess that means we have not 
had any warning flags set (because no such flags exist?)  to protect 
against the accidental introduction of the construct that this patch is 
testing compiler support for.

Best Wishes

Phillip