[PATCH] MyFirstContribution.txt: fix undeclared variable i in sample code

[PATCH] MyFirstContribution.txt: fix undeclared variable i in sample code

[Saksham Mittal]

In the sample code given to print the arguments given to ```git psuh```,
the iterating variable i is not declared as integer. I have fixed it so
the error is no longer there.

Signed-off-by: Saksham Mittal <gotlouemail@gmail.com>
---
 Documentation/MyFirstContribution.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/MyFirstContribution.txt b/Documentation/MyFirstContribution.txt
index 015cf24631..434a833a0b 100644
--- a/Documentation/MyFirstContribution.txt
+++ b/Documentation/MyFirstContribution.txt
@@ -297,7 +297,7 @@ existing `printf()` calls in place:
 		  "Your args (there are %d):\n",
 		  argc),
 	       argc);
-	for (i = 0; i < argc; i++)
+	for (int i = 0; i < argc; i++)
 		printf("%d: %s\n", i, argv[i]);
 
 	printf(_("Your current working directory:\n<top-level>%s%s\n"),
-- 
2.33.1

Re: [PATCH] MyFirstContribution.txt: fix undeclared variable i in sample code

[Johannes Altmanninger]

On Sat, Nov 13, 2021 at 05:58:35PM +0530, Saksham Mittal wrote:
> In the sample code given to print the arguments given to ```git psuh```,
> the iterating variable i is not declared as integer. I have fixed it so
> the error is no longer there.
> 
> Signed-off-by: Saksham Mittal <gotlouemail@gmail.com>
> ---
>  Documentation/MyFirstContribution.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/MyFirstContribution.txt b/Documentation/MyFirstContribution.txt
> index 015cf24631..434a833a0b 100644
> --- a/Documentation/MyFirstContribution.txt
> +++ b/Documentation/MyFirstContribution.txt
> @@ -297,7 +297,7 @@ existing `printf()` calls in place:
>  		  "Your args (there are %d):\n",
>  		  argc),
>  	       argc);
> -	for (i = 0; i < argc; i++)
> +	for (int i = 0; i < argc; i++)

It is declared, there is an "int i;" a few lines up.

Re: [PATCH] MyFirstContribution.txt: fix undeclared variable i in sample code

[Saksham Mittal]

> It is declared, there is an "int i;" a few lines up.

Oh, man, I never even saw that! The patch is completely unnecessary
then. Sorry for that!

Saksham

Re: [PATCH] MyFirstContribution.txt: fix undeclared variable i in sample code

[Junio C Hamano]

Saksham Mittal <gotlouemail@gmail.com> writes:

>> It is declared, there is an "int i;" a few lines up.
>
> Oh, man, I never even saw that! The patch is completely unnecessary
> then. Sorry for that!

No need to say sorry; you'd want to be a bit more careful next time,
that's all.

Also, our code does not introduce a new variable in the first part
of "for (;;)" loop control, so even if the original lacked decl for
"i", the posted patch is not how we write our code for this project.

Thanks.

Is 'for (int i = [...]' bad for C STD compliance reasons? (was: [PATCH] MyFirstContribution.txt: fix undeclared variable i in sample code)

[Ævar Arnfjörð Bjarmason]

On Sat, Nov 13 2021, Junio C Hamano wrote:

> Saksham Mittal <gotlouemail@gmail.com> writes:
>
>>> It is declared, there is an "int i;" a few lines up.
>>
>> Oh, man, I never even saw that! The patch is completely unnecessary
>> then. Sorry for that!
>
> No need to say sorry; you'd want to be a bit more careful next time,
> that's all.
>
> Also, our code does not introduce a new variable in the first part
> of "for (;;)" loop control, so even if the original lacked decl for
> "i", the posted patch is not how we write our code for this project.

Just curious: Out of preference, or for compatibility with older C
standards?

I'd think with the things we depend on in C99 it's probable that we
could start using this if standards conformance is the only obstacle.

But I haven't tested, so maybe I'm wrong, I'm just assuming that with
the C99 features we do have a hard dependency on surely anyone
implementing those would have implemented this too.

There's also a stylistic reason to avoid this pattern, i.e. some would
argue that it's better to declare variables up-front, since it tends to
encourage one to keep function definitions smaller (various in-tree
evidence to the contrary, but whatever).

I'd generally agree with that viewpoint & desire, but there's also cases
where being able to declare things in-line helps readability, e.g. when
your function needs two for-loops for some reason, they're set a bit
apart. Then the reader doesn't need to scan for whether an "i" is used
in-between the two.

I was thinking of the below code in bundle.c, I suppose some might find
the post-image less readable, but I remember starting to hunt around for
other out-of-loop uses of "i", which the post-image makes clear could be
avoided as far as variable scoping goes:

diff --git a/bundle.c b/bundle.c
index a0bb687b0f4..94edc186187 100644
--- a/bundle.c
+++ b/bundle.c
@@ -194,14 +194,14 @@ int verify_bundle(struct repository *r,
 	struct rev_info revs;
 	const char *argv[] = {NULL, "--all", NULL};
 	struct commit *commit;
-	int i, ret = 0, req_nr;
+	int ret = 0, req_nr;
 	const char *message = _("Repository lacks these prerequisite commits:");
 
 	if (!r || !r->objects || !r->objects->odb)
 		return error(_("need a repository to verify a bundle"));
 
 	repo_init_revisions(r, &revs, NULL);
-	for (i = 0; i < p->nr; i++) {
+	for (int i = 0; i < p->nr; i++) {
 		struct string_list_item *e = p->items + i;
 		const char *name = e->string;
 		struct object_id *oid = e->util;
@@ -223,12 +223,11 @@ int verify_bundle(struct repository *r,
 	if (prepare_revision_walk(&revs))
 		die(_("revision walk setup failed"));
 
-	i = req_nr;
-	while (i && (commit = get_revision(&revs)))
+	for (int i = req_nr; i && (commit = get_revision(&revs));)
 		if (commit->object.flags & PREREQ_MARK)
 			i--;
 
-	for (i = 0; i < p->nr; i++) {
+	for (int i = 0; i < p->nr; i++) {
 		struct string_list_item *e = p->items + i;
 		const char *name = e->string;
 		const struct object_id *oid = e->util;
@@ -242,7 +241,7 @@ int verify_bundle(struct repository *r,
 	}
 
 	/* Clean up objects used, as they will be reused. */
-	for (i = 0; i < p->nr; i++) {
+	for (int i = 0; i < p->nr; i++) {
 		struct string_list_item *e = p->items + i;
 		struct object_id *oid = e->util;
 		commit = lookup_commit_reference_gently(r, oid, 1);

Re: Is 'for (int i = [...]' bad for C STD compliance reasons?

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> Also, our code does not introduce a new variable in the first part
>> of "for (;;)" loop control, so even if the original lacked decl for
>> "i", the posted patch is not how we write our code for this project.
>
> Just curious: Out of preference, or for compatibility with older C
> standards?

The latter.

cc0c4297 (CodingGuidelines: spell out post-C89 rules, 2019-07-16)
adds a few "weather balloons say these are OK" together with this
exact one as "not yet allowed".  We (at least, those of us who have
enough knowledge and authority to propose changes to the guidelines)
all know that particular feature is a nice thing to use if everybody
we care about supports it [*1*].

Here is the thread that resulted in the relevant part of the
guideilne.

https://lore.kernel.org/git/CAPUEspgjSAqHUP2vsCCjqG8b0QkWdgoAByh4XdqsThQMt=V38w@mail.gmail.com/

The "another patch that tried to use it late last year" the thread
refers to is
https://lore.kernel.org/git/20181114004745.GH30222@szeder.dev/

If I am not mistaken, Carlo added gcc-4.8 CI job to catch these
recently?

Now, "Centos 6 is no longer" cannot be called a good response to
this message.  We stopped at seeing the first failure, and breakages
on other platforms were not even counted back then.  To those whose
compilers also barfed, it was sufficient that we pulled the plug
after seeing a failure on Centos 6.

But two years may be long enough for us to try again.  If we want to
pursue it, we'd need to raise a weather balloon that would break
compilers that have been happily grokking our code loudly by being
in a central place that will never be conditionally compiled out,
and is easy to back out by being in ultra-stable location.

cbc0f81d (strbuf: use designated initializers in STRBUF_INIT,
2017-07-10) is an example that Peff found and used a great such
location.

I know you are capable of reading Documentation/CodingGuidelines and
running "git blame" on it, and then use mailing list archive to dig
to find the answer, and it was a bit of disappointment to see this
was asked as a question, rather than a well researched "now after
two years, let's try this again".


[References]

*1* https://lore.kernel.org/git/xmqqlgnrq9qi.fsf@gitster.mtv.corp.google.com/

Re: Is 'for (int i = [...]' bad for C STD compliance reasons?

[Ævar Arnfjörð Bjarmason]

On Sun, Nov 14 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> Also, our code does not introduce a new variable in the first part
>>> of "for (;;)" loop control, so even if the original lacked decl for
>>> "i", the posted patch is not how we write our code for this project.
>>
>> Just curious: Out of preference, or for compatibility with older C
>> standards?
>
> The latter.
>
> cc0c4297 (CodingGuidelines: spell out post-C89 rules, 2019-07-16)
> adds a few "weather balloons say these are OK" together with this
> exact one as "not yet allowed".  We (at least, those of us who have
> enough knowledge and authority to propose changes to the guidelines)
> all know that particular feature is a nice thing to use if everybody
> we care about supports it [*1*].
>
> Here is the thread that resulted in the relevant part of the
> guideilne.
>
> https://lore.kernel.org/git/CAPUEspgjSAqHUP2vsCCjqG8b0QkWdgoAByh4XdqsThQMt=V38w@mail.gmail.com/
>
> The "another patch that tried to use it late last year" the thread
> refers to is
> https://lore.kernel.org/git/20181114004745.GH30222@szeder.dev/
>
> If I am not mistaken, Carlo added gcc-4.8 CI job to catch these
> recently?
>
> Now, "Centos 6 is no longer" cannot be called a good response to
> this message.  We stopped at seeing the first failure, and breakages
> on other platforms were not even counted back then.  To those whose
> compilers also barfed, it was sufficient that we pulled the plug
> after seeing a failure on Centos 6.
>
> But two years may be long enough for us to try again.  If we want to
> pursue it, we'd need to raise a weather balloon that would break
> compilers that have been happily grokking our code loudly by being
> in a central place that will never be conditionally compiled out,
> and is easy to back out by being in ultra-stable location.
>
> cbc0f81d (strbuf: use designated initializers in STRBUF_INIT,
> 2017-07-10) is an example that Peff found and used a great such
> location.
>
> I know you are capable of reading Documentation/CodingGuidelines and
> running "git blame" on it, and then use mailing list archive to dig
> to find the answer, and it was a bit of disappointment to see this
> was asked as a question, rather than a well researched "now after
> two years, let's try this again".
>
>
> [References]
>
> *1* https://lore.kernel.org/git/xmqqlgnrq9qi.fsf@gitster.mtv.corp.google.com/

The issue on CentOS 6 isn't one of incompatibility with C99, but that
the version of GCC refuses to compile C99 code without -std=c99 or
-std=gnu99. See [1] downthread of one of your links.

But yes, it would be the first C99 feature where we have a known
compiler that needs an opt-in -std=* option to support the C99 feature,
I think.

1. https://lore.kernel.org/git/20190717004231.GA93801@google.com/

Re: Is 'for (int i = [...]' bad for C STD compliance reasons?

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1178 bytes --]

On 2021-11-14 at 18:25:31, Ævar Arnfjörð Bjarmason wrote:
> The issue on CentOS 6 isn't one of incompatibility with C99, but that
> the version of GCC refuses to compile C99 code without -std=c99 or
> -std=gnu99. See [1] downthread of one of your links.
> 
> But yes, it would be the first C99 feature where we have a known
> compiler that needs an opt-in -std=* option to support the C99 feature,
> I think.

Yeah, as I've mentioned in the past, the impediment to C99 features is
MSVC.  All Unix compilers support it because it's obligatory for POSIX
1003.1-2001, and they have for some time, even if it's not the default
behavior.

MSVC recently learned C11, but I haven't fooled around with our CI
enough recently to see if I can get it to use C11.  I'll try to play
around some more.

I should also point out that CentOS 6 is now EOL and not receiving
security updates, and as such it shouldn't be a consideration in what we
do and don't support.  I think providing 10 years of support for an OS
in our project is already exceedingly generous, and most other projects
don't do so.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: Is 'for (int i = [...]' bad for C STD compliance reasons?

[Carlo Arenas]

On Sun, Nov 14, 2021 at 11:08 AM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> MSVC recently learned C11, but I haven't fooled around with our CI
> enough recently to see if I can get it to use C11.  I'll try to play
> around some more.

The last 2 releases support C17, but I think that is only using the
universal crt which is not what git for windows link with and might
not be available by default (10 or later feature) in all windows
versions they target as well.

Carlo

PS. no Windows expert at all, and I am sure more authoritative answers
will come along, just wanted to let you know to probably avoid wasting
your time with the CI

Re: Is 'for (int i = [...]' bad for C STD compliance reasons?

[Carlo Arenas]

On Sun, Nov 14, 2021 at 10:31 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> The issue on CentOS 6 isn't one of incompatibility with C99, but that
> the version of GCC refuses to compile C99 code without -std=c99 or
> -std=gnu99. See [1] downthread of one of your links.

FWIW while CentOS 6 is EOL, CentOS 7 (gcc 4.8.5) is also affected and
has at least one more year of "support".

You are correct that without a specific -std flag the build will
break, and unlike what is expected from all other C99 features that
were supported by gnu89 (the default until gcc >= 5) and that are
currently in use.  The fact that the pedantic rollout went smoothly is
encouraging in that respect, but take into consideration that is also
limited only to DEVELOPER=1.

Carlo

PS. there is a CI job in travis but travis is dead

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

[Junio C Hamano]

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

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

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

[Martin Ågren]

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

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

[Junio C Hamano]

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.

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2188 bytes --]

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
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[Phillip Wood]

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);
>   }
> 

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

[Ævar Arnfjörð Bjarmason]

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.

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

[SZEDER Gábor]

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

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

[Junio C Hamano]

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.

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

[Phillip Wood]

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

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

[Junio C Hamano]

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.

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

[Ævar Arnfjörð Bjarmason]

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.

Re: Removing -Wdeclaration-after-statement

[Junio C Hamano]

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