Removing -Wdeclaration-after-statement (was: [PATCH] revision: use C99 declaration of variable in for() loop)

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Nov 17 2021, 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, 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.

Why would that be sad? The intent of -Wdeclaration-after-statement is to
catch C90 compatibility issues. Maybe we don't want to enable everything
C99-related in this area at once, but why shouldn't we be removing
-Wdeclaration-after-statement once we have a hard C99 dependency?

I usually prefer declaring variables up-front just as a metter of style,
and it usually encourages you to split up functions that are
unnecessarily long.

But I think being able to do it in some situations also helps
readability. E.g. I'm re-rolling my cat-file usage topic now and spotted
this nice candidate (which we'd error on now with CC=gcc and
DEVELOPER=1):
	
	diff --git a/builtin/cat-file.c b/builtin/cat-file.c
	index f5437c2d045..a43df23a7cd 100644
	--- a/builtin/cat-file.c
	+++ b/builtin/cat-file.c
	@@ -644,8 +644,6 @@ static int batch_option_callback(const struct option *opt,
	 int cmd_cat_file(int argc, const char **argv, const char *prefix)
	 {
	 	int opt = 0;
	-	int opt_cw = 0;
	-	int opt_epts = 0;
	 	const char *exp_type = NULL, *obj_name = NULL;
	 	struct batch_options batch = {0};
	 	int unknown_type = 0;
	@@ -708,8 +706,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
	 	batch.buffer_output = -1;
	 
	 	argc = parse_options(argc, argv, prefix, options, usage, 0);
	-	opt_cw = (opt == 'c' || opt == 'w');
	-	opt_epts = (opt == 'e' || opt == 'p' || opt == 't' || opt == 's');
	+	const int opt_cw = (opt == 'c' || opt == 'w');
	+	const opt_epts = (opt == 'e' || opt == 'p' || opt == 't' || opt == 's');
	 
	 	/* --batch-all-objects? */
	 	if (opt == 'b')

I.e. in this case I'm declaring a variable merely as a short-hand for
accessing "opt", and due to the need for parse_options() we can't really
declare it in a way that's resonable before any statement in the
function.

By having -Wdeclaration-after-statement we're forced to make it
non-const, and having it "const" helps readability, you know as soon as
you see it that it won't be modified.

That particular example is certainly open to bikeshedding, but I think
the general point that it's not categorically bad holds, and therefore
if we don't need it for compiler compatibility it's probably a good idea
to allow it.