Is 'for (int i = [...]' bad for C STD compliance reasons? (was: [PATCH] MyFirstContribution.txt: fix undeclared variable i in sample code)
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Nov 14, 2021, 14:28 UTC
- Message-ID
- <211114.868rxqu7hr.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <xmqq7ddbme7q.fsf@gitster.g>
On Sat, Nov 13 2021, Junio C Hamano wrote:
Show 13 quoted lines
> 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);