Phillip Wood [off-list ref] 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.
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.
On 18/11/2021 07:09, Junio C Hamano wrote:
Phillip Wood [off-list ref] writes:
quoted
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
On Wed, Nov 17 2021, Junio C Hamano wrote:
Phillip Wood [off-list ref] writes:
quoted
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.