Is 'for (int i = [...]' bad for C STD compliance reasons? (was: [PATCH] MyFirstContribution.txt: fix undeclared variable i in sample code)
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:
Show changes to bundle.c +5 −6
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);
