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

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 <redacted>
---
 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

On Mon, 15 Nov 2021 at 07:30, Junio C Hamano [off-list ref] 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.

-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

Martin Ågren [off-list ref] writes:

On Mon, 15 Nov 2021 at 07:30, Junio C Hamano [off-list ref] wrote:

quoted

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.

quoted

is a long-enough time, so let's try it agin.

s/agin/again/

quoted

 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

On Wed, Nov 17 2021, Phillip Wood wrote:

Hi Junio

On 15/11/2021 06:27, Junio C Hamano wrote:

quoted

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:

quoted

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).