| Message ID | xmqqpmr2j5lq.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | revision: use C99 declaration of variable in for() loop | expand |
On Mon, 15 Nov 2021 at 07:30, Junio C Hamano <gitster@pobox.com> wrote: > > There are certain C99 features that might be nice to use in our code > base, but we've hesitated to do so in order to avoid breaking > compatibility with older compilers. But we don't actually know if > people are even using pre-C99 compilers these days. > is a long-enough time, so let's try it agin. s/agin/again/ > void show_object_with_name(FILE *out, struct object *obj, const char *name) > { > - const char *p; > - > fprintf(out, "%s ", oid_to_hex(&obj->oid)); > - for (p = name; *p && *p != '\n'; p++) > + for (const char *p = name; *p && *p != '\n'; p++) > fputc(*p, out); > fputc('\n', out); > } This seems like a stable-enough function for this experiment. Similar to 765dc16888 ("git-compat-util: always enable variadic macros", 2021-01-28), maybe we should add something like /* * This "for (const char *p = ..." is made as a first step towards * making use of such declarations elsewhere in our codebase. If * it causes compilation problems on your platform, please report * it to the Git mailing list at git@vger.kernel.org. */ to reduce the chance of someone patching it up locally thinking that it's just a one-off. Martin
On 2021-11-15 at 06:27:45, Junio C Hamano wrote: > There are certain C99 features that might be nice to use in our code > base, but we've hesitated to do so in order to avoid breaking > compatibility with older compilers. But we don't actually know if > people are even using pre-C99 compilers these days. > > One way to figure that out is to introduce a very small use of a > feature, and see if anybody complains, and we've done so to probe > the portability for a few features like "trailing comma in enum > declaration", "designated initializer for struct", and "designated > initializer for array". A few years ago, we tried to use a handy > > for (int i = 0; i < n; i++) > use(i); > > to introduce a new variable valid only in the loop, but found that > some compilers we cared about didn't like it back then. Two years > is a long-enough time, so let's try it agin. I think you absolutely need a compiler option for this to work on older systems. Many of those compilers support C99 just fine but need an option to enable it. I think this could go on top of my patch, though. > If this patch can survive a few releases without complaint, then we > can feel more confident that variable declaration in for() loop is > supported by the compilers our user base use. And if we do get > complaints, then we'll have gained some data and we can easily > revert this patch. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > revision.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/revision.c b/revision.c > index 9dff845bed..44492f2c02 100644 > --- a/revision.c > +++ b/revision.c > @@ -43,10 +43,8 @@ static inline int want_ancestry(const struct rev_info *revs); > > void show_object_with_name(FILE *out, struct object *obj, const char *name) > { > - const char *p; > - > fprintf(out, "%s ", oid_to_hex(&obj->oid)); > - for (p = name; *p && *p != '\n'; p++) > + for (const char *p = name; *p && *p != '\n'; p++) > fputc(*p, out); > fputc('\n', out); > } > -- > 2.34.0-rc2-165-g9b3c04af29 >
Martin Ågren <martin.agren@gmail.com> writes: > On Mon, 15 Nov 2021 at 07:30, Junio C Hamano <gitster@pobox.com> wrote: >> >> There are certain C99 features that might be nice to use in our code >> base, but we've hesitated to do so in order to avoid breaking >> compatibility with older compilers. But we don't actually know if >> people are even using pre-C99 compilers these days. > >> is a long-enough time, so let's try it agin. > > s/agin/again/ > >> void show_object_with_name(FILE *out, struct object *obj, const char *name) >> { >> - const char *p; >> - >> fprintf(out, "%s ", oid_to_hex(&obj->oid)); >> - for (p = name; *p && *p != '\n'; p++) >> + for (const char *p = name; *p && *p != '\n'; p++) >> fputc(*p, out); >> fputc('\n', out); >> } > > This seems like a stable-enough function for this experiment. Yup, the callers and the implementation are from several years ago, if I am not mistaken. > Similar to 765dc16888 ("git-compat-util: always enable variadic macros", > 2021-01-28), maybe we should add something like > > /* > * This "for (const char *p = ..." is made as a first step towards > * making use of such declarations elsewhere in our codebase. If > * it causes compilation problems on your platform, please report > * it to the Git mailing list at git@vger.kernel.org. > */ > > to reduce the chance of someone patching it up locally thinking that > it's just a one-off. Probably. It would help people to refrain from copying and pasting this and making it harder to back out, too.
Hi Junio On 15/11/2021 06:27, Junio C Hamano wrote: > There are certain C99 features that might be nice to use in our code > base, but we've hesitated to do so in order to avoid breaking > compatibility with older compilers. But we don't actually know if > people are even using pre-C99 compilers these days. > > One way to figure that out is to introduce a very small use of a > feature, and see if anybody complains, and we've done so to probe > the portability for a few features like "trailing comma in enum > declaration", "designated initializer for struct", and "designated > initializer for array". A few years ago, we tried to use a handy > > for (int i = 0; i < n; i++) > use(i); > > to introduce a new variable valid only in the loop, but found that > some compilers we cared about didn't like it back then. Two years > is a long-enough time, so let's try it agin. > > If this patch can survive a few releases without complaint, then we > can feel more confident that variable declaration in for() loop is > supported by the compilers our user base use. And if we do get > complaints, then we'll have gained some data and we can easily > revert this patch. 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 we could wrap the loop in gcc's warning pragmas to avoid that. Best Wishes Phillip > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > revision.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/revision.c b/revision.c > index 9dff845bed..44492f2c02 100644 > --- a/revision.c > +++ b/revision.c > @@ -43,10 +43,8 @@ static inline int want_ancestry(const struct rev_info *revs); > > void show_object_with_name(FILE *out, struct object *obj, const char *name) > { > - const char *p; > - > fprintf(out, "%s ", oid_to_hex(&obj->oid)); > - for (p = name; *p && *p != '\n'; p++) > + for (const char *p = name; *p && *p != '\n'; p++) > fputc(*p, out); > fputc('\n', out); > } >
On Wed, Nov 17 2021, Phillip Wood wrote: > Hi Junio > > On 15/11/2021 06:27, Junio C Hamano wrote: >> There are certain C99 features that might be nice to use in our code >> base, but we've hesitated to do so in order to avoid breaking >> compatibility with older compilers. But we don't actually know if >> people are even using pre-C99 compilers these days. >> One way to figure that out is to introduce a very small use of a >> feature, and see if anybody complains, and we've done so to probe >> the portability for a few features like "trailing comma in enum >> declaration", "designated initializer for struct", and "designated >> initializer for array". A few years ago, we tried to use a handy >> for (int i = 0; i < n; i++) >> use(i); >> to introduce a new variable valid only in the loop, but found that >> some compilers we cared about didn't like it back then. Two years >> is a long-enough time, so let's try it agin. >> If this patch can survive a few releases without complaint, then we >> can feel more confident that variable declaration in for() loop is >> supported by the compilers our user base use. And if we do get >> complaints, then we'll have gained some data and we can easily >> revert this patch. > > 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 we could wrap the loop in gcc's warning pragmas to avoid > that. Good point. Overall something that brings us to the end-state 765dc168882 (git-compat-util: always enable variadic macros, 2021-01-28) brought us to is probably better, i.e. something you can work around by defining or undefining a macro via a Makefile parameter, instead of needing to patch git's sources.
On Wed, Nov 17, 2021 at 11:03:58AM +0000, Phillip Wood wrote: > Hi Junio > > On 15/11/2021 06:27, Junio C Hamano wrote: > > There are certain C99 features that might be nice to use in our code > > base, but we've hesitated to do so in order to avoid breaking > > compatibility with older compilers. But we don't actually know if > > people are even using pre-C99 compilers these days. > > > > One way to figure that out is to introduce a very small use of a > > feature, and see if anybody complains, and we've done so to probe > > the portability for a few features like "trailing comma in enum > > declaration", "designated initializer for struct", and "designated > > initializer for array". A few years ago, we tried to use a handy > > > > for (int i = 0; i < n; i++) > > use(i); > > > > to introduce a new variable valid only in the loop, but found that > > some compilers we cared about didn't like it back then. Two years > > is a long-enough time, so let's try it agin. > > > > If this patch can survive a few releases without complaint, then we > > can feel more confident that variable declaration in for() loop is > > supported by the compilers our user base use. And if we do get > > complaints, then we'll have gained some data and we can easily > > revert this patch. > > 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 > we could wrap the loop in gcc's warning pragmas to avoid that. The scope of the loop variable is limited to the loop, so I don't think this is considered as declaration after statement, just like other variable declarations in limited scopes that are abundant in Git's codebase, e.g.: printf("..."); if (var) { int a; ... } FWIW, I've spent some time with Compiler Explorer compiling a for loop initial declaration after a statement with '-std=c99 -Werror -Wdeclaration-after-statement', and none of them complained (though there were some that didn't understand the '-std=c99' or '-Wdecl...' options or couldn't compile it for some other reason).
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. 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 <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
Phillip Wood <phillip.wood123@gmail.com> writes: > ... 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. I think we actually saw some instances of "for (type var = ..." slipped through the review process, only to later get caught by some other means.
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.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > 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? We already heard from people that we do not want vla, and I agree that we do not want all C99. decl-after-stmt is something I definitely do not want in our code, in order to keep the code more readable by declaring the things that will be used in the scope upfront, with documentation if needed. It tends to encourage us to keep our blocks smaller.
diff --git a/revision.c b/revision.c index 9dff845bed..44492f2c02 100644 --- a/revision.c +++ b/revision.c @@ -43,10 +43,8 @@ static inline int want_ancestry(const struct rev_info *revs); void show_object_with_name(FILE *out, struct object *obj, const char *name) { - const char *p; - fprintf(out, "%s ", oid_to_hex(&obj->oid)); - for (p = name; *p && *p != '\n'; p++) + for (const char *p = name; *p && *p != '\n'; p++) fputc(*p, out); fputc('\n', out); }
There are certain C99 features that might be nice to use in our code base, but we've hesitated to do so in order to avoid breaking compatibility with older compilers. But we don't actually know if people are even using pre-C99 compilers these days. One way to figure that out is to introduce a very small use of a feature, and see if anybody complains, and we've done so to probe the portability for a few features like "trailing comma in enum declaration", "designated initializer for struct", and "designated initializer for array". A few years ago, we tried to use a handy for (int i = 0; i < n; i++) use(i); to introduce a new variable valid only in the loop, but found that some compilers we cared about didn't like it back then. Two years is a long-enough time, so let's try it agin. If this patch can survive a few releases without complaint, then we can feel more confident that variable declaration in for() loop is supported by the compilers our user base use. And if we do get complaints, then we'll have gained some data and we can easily revert this patch. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- revision.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)