Removing -Wdeclaration-after-statement (was: [PATCH] revision: use C99 declaration of variable in for() loop)
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Dec 8, 2021, 12:17 UTC
- Message-ID
- <211208.86wnkfl1ni.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <xmqq1r3eym7f.fsf@gitster.g>
On Wed, Nov 17 2021, Junio C Hamano wrote:
Show 40 quoted lines
> 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.