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" <avarab@gmail.com>]

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